Add error handling to the API blueprint. by scolby33 · Pull Request #40 · scolby33/OCSPdash

scolby33 · 2018-08-07T09:20:10Z

Will close #25.

codecov · 2018-08-07T09:41:06Z

Codecov Report

Merging #40 into develop will increase coverage by 8.82%.
The diff coverage is 24.27%.

@@             Coverage Diff             @@
##           develop      #40      +/-   ##
===========================================
+ Coverage     42.9%   51.73%   +8.82%     
===========================================
  Files           20       21       +1     
  Lines          825      893      +68     
  Branches        75       79       +4     
===========================================
+ Hits           354      462     +108     
+ Misses         461      418      -43     
- Partials        10       13       +3

Impacted Files	Coverage Δ
src/ocspdash/models.py	`77.14% <ø> (ø)`	⬆️
src/ocspdash/web/app.py	`86.66% <100%> (+86.66%)`	⬆️
src/ocspdash/web/blueprints/api.py	`29.52% <12.32%> (+29.52%)`	⬆️
src/ocspdash/web/exceptions.py	`43.75% <43.75%> (ø)`
src/ocspdash/manager.py	`63.08% <50%> (+0.75%)`	⬆️
src/ocspdash/web/extension.py	`93.33% <66.66%> (+93.33%)`	⬆️
src/ocspdash/web/admin.py	`43.75% <0%> (+43.75%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4472751...fcfdcee. Read the comment docs.

Start of work on #25.

cthoyt

Overall looks good!

Requires lots more unit testing, though. And having everything go through 400 makes it tricky to pick out that the right error comes at the right time (besides doing string checking)

cthoyt · 2018-08-07T14:04:12Z

-    prepared_result_dicts = (_prepare_result_dictionary(result_data)
-                             for result_data in claims[OCSP_RESULTS_JWT_CLAIM])
+    try:
+        prepared_result_dicts = (_prepare_result_dictionary(result_data)


Come on, use proper spacing :)

What did I do wrong here?

cthoyt · 2018-08-07T14:04:38Z

+    except (KeyError, ValueError):
+        abort(HTTPStatus.BAD_REQUEST, 'invalid result data')
+
+    # TODO: can this raise an exception? I think yes if there's a constraint broken on the DB when commit is called()


catch an integrity error then

cthoyt · 2018-08-07T14:05:58Z

    n = request.args.get('n', type=int, default=10)  # TODO make configurable at app level
    if n > 10:
-        abort(400, 'n too large, max is 10')  # TODO get the max config value here too
+        abort(HTTPStatus.BAD_REQUEST, 'n too large, max is 10')  # TODO get the max config value here too


is there a wrapper for aborting on 400?

No, the flask docs have things like abort(404). It is annoying to be so much wordier with HTTPStatus.BAD_REQUEST but I think that's better than a "magic" number even if most everyone knows it.

cthoyt · 2018-08-07T14:06:16Z

+        invite_token = b64decode(claims['token'])
+    except KeyError:
+        abort(HTTPStatus.BAD_REQUEST, "'token' missing from claims")
+    except binascii.Error:


that's an awful name for an error

I take what the stdlib provides.

namespaces, those are a honking good thing

scolby33 · 2018-08-07T18:54:45Z

I agree so many 400's is not the best, but none of the other error codes seem to fit better.

One thing I have seen is returning JSON errors like (for Pushover, for example):

{
  "user": "invalid",
  "errors": [
    "user identifier is invalid"
  ],
  "status": 0,
  "request": "5042853c-402d-4a18-abcb-168734a801de"
}

Despite them having a list of errors, I don't think I ever saw a response with more than one there.

cthoyt · 2018-08-07T19:33:53Z

You can return json and give it a non-200 error

scolby33 · 2018-08-08T07:09:02Z

Well, gonna try this now http://flask.pocoo.org/docs/1.0/patterns/apierrors/

http://flask.pocoo.org/docs/1.0/patterns/apierrors/ Changes for flake and mypy.

- Add client_session and client_function fixtures to create Flask test clients and rollback the db between tests - Add get_db staticmethod to OCSPSQLAlchemy class - Add the ability to pass session_options into the flask-sqlalchemy SQLAlchemy constructor from the create_application function so the test fixtures can take control of rollbacks. - Add stub test to start using the test client.

…type annotation.

scolby33 · 2018-08-08T10:02:48Z

+    logger.debug('rolling back transaction from function for web client')
+    transaction.rollback()
+



@cthoyt if you can figure out how to reduce the repetition between manager_session and client_session and between manager_function and client_function that'd be swell.

But that's not your real homework.

scolby33 · 2018-08-08T10:04:14Z

+    transaction.rollback()
+

+# TODO: fixture to pre-fill DB with some stuff for the client to test on


@cthoyt this is the real homework.

Something that takes in a client and spits out the same client with a db with a bunch of stuff in it.

It'd be cool if it didn't get rolled back, but that might be a huge pain so whatever works.

Scoped sessions act like sessions... why don't they just inherit from them?!?

The SQLAlchemy class takes a none, so you can pass it none (unless i'm an idiot and none means something special)

@scolby33

@scolby33 not sure where you want this, but here's the idea with making some test data

scolby33 · 2018-08-08T17:42:52Z

        location = self.get_location_by_selector(selector)
        if location is None:
-            raise Exception(f'location not found for selector: {selector}')
+            raise ValueError(f'invalid invite token')


Are these vexing exceptions? @cthoyt

No, since it doesn't mean your code has been designed wrong, but rather you have to deal with the reality that someone else is going to stick weird shit inside as the invite token.

Now, the next exception checking for the expiration of the pubkey might be different, but raising an exception actually makes the logic for using this function much better. Overall, I like this design and I think it does what it's supposed to in an elegant way. Lots of helpful/useful errors for bad situations where the program should stop seems good to me.

cthoyt · 2018-08-12T21:14:19Z

-    except jwt.JWTError:
-        abort(HTTPStatus.BAD_REQUEST, 'malformed JWT')
+    except jwt.JWTError as e:
+        raise InvalidUsage(f'failed to decode JWT: {str(e)}')


you could also do

raise InvalidUsage('failed to decode JWT') from e

unless the point of this was to get all of that information in the string

cthoyt · 2018-08-12T21:15:10Z

+    except ValueError as e:
+        raise InvalidUsage(f'failed to process invite: {str(e)}')

    return '', HTTPStatus.NO_CONTENT


I still don't like returning no content... why don't you have it send back some information about the location that's been registered?

201 Created specifies you should return the URL of the created entity but there is no URL for the location.

What would you want to return? The location name? I was originally just jsonifying the location, but there's some fields that probably shouldn't be returned and going down the road of to_json and safe_to_json screams bad to me.

- Use app configuration for default and maximum manifest size - Handle integrity error when inserting payload - Also add raising of integrity errors to documentation - make secret key more secret

cthoyt · 2018-08-31T21:44:46Z

yo whats going on with this PR

cthoyt · 2018-10-07T20:10:54Z

BUMP

scolby33 requested a review from cthoyt August 7, 2018 09:20

scolby33 added 5 commits August 7, 2018 02:48

Add error handling to /register API endpoint.

60b9e20

Start of work on #25.

Add :raises: to docstrings where appropriate.

5a5d3a0

Ignore McCabe complexity on register_location_key.

7901d13

Add more error checking to the API.

92b4709

Ignore McCabe complexity on submit.

a686985

scolby33 force-pushed the error-handling branch from b6528de to a686985 Compare August 7, 2018 09:49

cthoyt approved these changes Aug 7, 2018

View reviewed changes

scolby33 added 8 commits August 8, 2018 00:45

Switch to using an Exception instead of manually calling abort()

b0defea

http://flask.pocoo.org/docs/1.0/patterns/apierrors/ Changes for flake and mypy.

Remove some print() debugging. Sorry @cthoyt

6318848

Fancier error messages for the API.

83b2850

Add status key to the response JSON of InvalidUsage exception.

e9e86e9

Broken client_session fixture first try.

2cbb4c9

Style fixes.

efbf402

Since this static method is kinda generic, use a more generic return …

a67cb91

…type annotation.

scolby33 commented Aug 8, 2018

View reviewed changes

scolby33 changed the title ~~Add error handling to /register API endpoint.~~ Add error handling to the API blueprint. Aug 8, 2018

cthoyt added 4 commits August 8, 2018 18:34

Update type hints and documentation

cc4281f

Scoped sessions act like sessions... why don't they just inherit from them?!?

Remove unnecessary default

5d4cf22

The SQLAlchemy class takes a none, so you can pass it none (unless i'm an idiot and none means something special)

Combine logic

e5eea9a

Add function to make some test data

bb2b14a

@scolby33 not sure where you want this, but here's the idea with making some test data

scolby33 commented Aug 8, 2018

View reviewed changes

cthoyt reviewed Aug 12, 2018

View reviewed changes

Address TODOs

fcfdcee

- Use app configuration for default and maximum manifest size - Handle integrity error when inserting payload - Also add raising of integrity errors to documentation - make secret key more secret

		logger.debug('rolling back transaction from function for web client')
		transaction.rollback()

		transaction.rollback()


		# TODO: fixture to pre-fill DB with some stuff for the client to test on

Conversation

scolby33 commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cthoyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scolby33 commented Aug 7, 2018

Uh oh!

cthoyt commented Aug 7, 2018

Uh oh!

scolby33 commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scolby33 Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cthoyt commented Aug 31, 2018

Uh oh!

cthoyt commented Oct 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scolby33 commented Aug 7, 2018 •

edited

Loading

codecov Bot commented Aug 7, 2018 •

edited

Loading

scolby33 Aug 8, 2018 •

edited

Loading