Add type annotations to journalist_app.api #5464

nabla-c0d3 · 2020-08-22T05:03:14Z

Status

Ready for review.

Description of Changes

To help with #4399 , this PR adds type annotations to journalist_app/api.py

nabla-c0d3 · 2020-08-22T05:19:39Z

securedrop/journalist_app/api.py

-    auth_token = request.headers.get('Authorization').split(" ")[1]
-    user = Journalist.validate_api_token_and_get_user(auth_token)
-    return user
+def _authenticate_user_from_auth_header(request: flask.Request) -> Journalist:


There was code to parse the Authorization header in both get_user_object() and token_required(), so i combined them into just one function (because get_user_object() was causing a lot typing errors).

I would suggest passing the resulting/authenticated user as an argument to functions decorated with @token_require so that the views that need to know who the current user is don't need to parse the Auth header again (as done in all_source_replies() for example).
I can make the change if that's useful.

It would generally be better to make functional changes in separate PRs, particularly when they're optimizations and involve authentication, instead of mixing the refactoring with the type hinting.

I preferred the old name, get_user_object: it was accurate, if less precise, and a lot shorter.

kushaldas

At least for 3 people, including my local laptop: the branch is throwing this following error:

✦ ❯ make  typelint
███ Running mypy type checking...
securedrop/journalist_app/api.py:200: error: Incompatible return value type (got "werkzeug.wrappers.Response", expected "flask.wrappers.Response")
securedrop/journalist_app/api.py:209: error: Incompatible return value type (got "werkzeug.wrappers.Response", expected "flask.wrappers.Response")
Found 2 errors in 1 file (checked 104 source files)
make: *** [Makefile:124: typelint] Error 1

kushaldas · 2020-09-15T12:45:05Z

@emkll please have a look at this, I am okay to approve, but want to know why a few of us saw that error.

nabla-c0d3 · 2020-09-15T12:46:14Z

Didn't have this error locally but I can fix it anyway, later today or tomorrow.

kushaldas · 2020-09-15T12:48:41Z

Didn't have this error locally but I can fix it anyway, later today or tomorrow.

Yes, I can see this in my laptop, but not in the other places. I can not even see this in the CI. You don't have to change anything right now. But, we should figure out why are we seeing this in some machines.

emkll · 2020-09-15T13:56:20Z

Strange, I too am seeing the same error @kushaldas is describing. Those errors occur when running make typelint locally, either in a virtual environment or in the dev container. Looks like typelint is passing in CI. I double checked the version, they are the same in both CI and in my local virtualenv:

mypy==0.761
mypy-extensions==0.4.3

We should definitely resolve this discrepancy prior to merging.

@nabla-c0d3 if you want to take a stab at resolving the issue, that would be greatly appreciated. Otherwise we would be happy to investigate on your behalf, just let us know. Thank you for your continued contributions to the project!

nabla-c0d3 · 2020-09-19T09:07:22Z

I haven't been able to reproduce this error. I do get the same mypy error message if I replace:

def serve_file_with_etag(db_obj: Union[Reply, Submission]) -> flask.Response:

with

def serve_file_with_etag(db_obj: Union[Reply, Submission]) -> werkzeug.Response:

which then returns the error:

$ make typelint
Running mypy type checking...
securedrop/journalist_app/api.py:200: error: Incompatible return value type (got "werkzeug.wrappers.Response", expected "flask.wrappers.Response")
securedrop/journalist_app/api.py:209: error: Incompatible return value type (got "werkzeug.wrappers.Response", expected "flask.wrappers.Response")
Found 2 errors in 1 file (checked 104 source files)
make: *** [typelint] Error 1

However this branch does have the correct flask.Response as the return type, which doesn't trigger the error in CI and on my computer.

nabla-c0d3 · 2020-09-20T08:32:04Z

I would suggest merging this PR (so I can continue adding type annotations elsewhere), and then investigating as part of #5516.

emkll · 2020-09-21T15:57:48Z

Sorry for the delay @nabla-c0d3 . The source of the confusion on my end is likely related to the merge commit that included this change as part of conflict resolution in a041434

I've taken the liberty of squashing/rebasing this branch on latest develop (removing merge commits) for a final round of CI. make typelint is now passing locally and in https://github.com/freedomofpress/securedrop/compare/rebase-5464

emkll

Thanks for the changes and for your patience @nabla-c0d3 the changes looks good to me, with linting passing locally and CI still passing.

To aid the review of future PR's, it would be helpful if you could:

Squash/rebase on latest develop instead of merging into your branch (though we would be happy to rebase on your behalf, let us know if you'd prefer to do it yourself)
Either limit or separate code refactoring from type annotations as much as possible

Thanks again for your contributions! We'll take a look at #5491 next.

emkll · 2020-09-21T16:00:51Z

securedrop/journalist_app/api.py

-        result = model.query.filter(column == object_id).one_or_none()
-    else:
-        result = model.query.get(object_id)
+def get_or_404(model: db.Model, object_id: str, column: str) -> db.Model:


Looks like we always specify column as parameter here, so lgtm

rmol · 2020-09-21T19:21:12Z

securedrop/journalist_app/api.py

-    auth_token = request.headers.get('Authorization').split(" ")[1]
-    user = Journalist.validate_api_token_and_get_user(auth_token)
-    return user
+def _authenticate_user_from_auth_header(request: flask.Request) -> Journalist:


It would generally be better to make functional changes in separate PRs, particularly when they're optimizations and involve authentication, instead of mixing the refactoring with the type hinting.

I preferred the old name, get_user_object: it was accurate, if less precise, and a lot shorter.

rmol · 2020-09-21T19:21:36Z

securedrop/journalist_app/api.py

-        result = model.query.filter(column == object_id).one_or_none()
-    else:
-        result = model.query.get(object_id)
+def get_or_404(model: db.Model, object_id: str, column: str) -> db.Model:


rmol · 2020-09-21T19:25:13Z

securedrop/journalist_app/api.py

        if request.method == 'GET':
            source = get_or_404(Source, source_uuid, column=Source.uuid)
            return jsonify(source.to_json()), 200
        elif request.method == 'DELETE':
            source = get_or_404(Source, source_uuid, column=Source.uuid)
            utils.delete_collection(source.filesystem_id)
            return jsonify({'message': 'Source and submissions deleted'}), 200
+        else:
+            abort(404)


This is a good addition, as Flask's implicit support for HEAD means we're setting up a 500 error when we fall out of the if without handling it, but I think a 405 (method not allowed) status would be more accurate here and the other places you've caught this.

nabla-c0d3 commented Aug 22, 2020

View reviewed changes

nabla-c0d3 marked this pull request as ready for review September 4, 2020 07:22

nabla-c0d3 requested review from kushaldas and redshiftzero as code owners September 4, 2020 07:22

nabla-c0d3 mentioned this pull request Sep 12, 2020

Finish adding type annotations to journalist_app #5491

Merged

kushaldas reviewed Sep 15, 2020

View reviewed changes

emkll mentioned this pull request Sep 18, 2020

Investigate and resolve MyPy discreptency in CI / local environments #5516

Closed

Add type annotations to journalist_app.api

5ae517b

emkll force-pushed the add-types-to-api-in-journalist-app branch from a2b899c to 5ae517b Compare September 21, 2020 15:57

emkll approved these changes Sep 21, 2020

View reviewed changes

rmol reviewed Sep 21, 2020

View reviewed changes

emkll merged commit 98153cf into freedomofpress:develop Sep 21, 2020

nabla-c0d3 deleted the add-types-to-api-in-journalist-app branch October 24, 2020 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type annotations to journalist_app.api #5464

Add type annotations to journalist_app.api #5464

nabla-c0d3 commented Aug 22, 2020 •

edited

Loading

nabla-c0d3 Aug 22, 2020 •

edited

Loading

rmol Sep 21, 2020

kushaldas left a comment

kushaldas commented Sep 15, 2020

nabla-c0d3 commented Sep 15, 2020

kushaldas commented Sep 15, 2020

emkll commented Sep 15, 2020

nabla-c0d3 commented Sep 19, 2020

nabla-c0d3 commented Sep 20, 2020

emkll commented Sep 21, 2020

emkll left a comment

emkll Sep 21, 2020

rmol Sep 21, 2020

rmol Sep 21, 2020

rmol Sep 21, 2020

rmol Sep 21, 2020

Add type annotations to journalist_app.api #5464

Add type annotations to journalist_app.api #5464

Conversation

nabla-c0d3 commented Aug 22, 2020 • edited Loading

Status

Description of Changes

nabla-c0d3 Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

rmol Sep 21, 2020

Choose a reason for hiding this comment

kushaldas left a comment

Choose a reason for hiding this comment

kushaldas commented Sep 15, 2020

nabla-c0d3 commented Sep 15, 2020

kushaldas commented Sep 15, 2020

emkll commented Sep 15, 2020

nabla-c0d3 commented Sep 19, 2020

nabla-c0d3 commented Sep 20, 2020

emkll commented Sep 21, 2020

emkll left a comment

Choose a reason for hiding this comment

emkll Sep 21, 2020

Choose a reason for hiding this comment

rmol Sep 21, 2020

Choose a reason for hiding this comment

rmol Sep 21, 2020

Choose a reason for hiding this comment

rmol Sep 21, 2020

Choose a reason for hiding this comment

rmol Sep 21, 2020

Choose a reason for hiding this comment

nabla-c0d3 commented Aug 22, 2020 •

edited

Loading

nabla-c0d3 Aug 22, 2020 •

edited

Loading