Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement 553 Version Not Supported #420

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Jul 21, 2020

Closes #391

New middleware to check whether versioned base URL (relating to the OPTIMADE server) is a valid version.
If it is not a valid version, the middleware will raise, resulting in a 553 Version Not Supported as required by the specification.

Note: This PR changes test_config.json to align the base_url configuration parameter with the used test client's.
This speaks to the fact that the default_config.json and example_config.json should be usable as proper configuration files for development of this package.

This also updates a bit of CI now:
- Use v1 of CasperWA/push-protected action.
- Change job name deps_static to pytest.
- Use a high verbosity for pytest, i.e., -vvv.

Edit: These updates have been factored out into another PR, since they are out-of-scope for this PR. See #422.

@CasperWA CasperWA requested review from ml-evs and shyamd July 21, 2020 15:33
@CasperWA
Copy link
Member Author

Don't know why the tests are failing here. Locally they run fine for me.

@CasperWA CasperWA changed the title Implement 533 Version Not Supported Implement 553 Version Not Supported Jul 21, 2020
@CasperWA
Copy link
Member Author

Gah - just realized it's 553 not 533...

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #420 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   90.99%   91.09%   +0.09%     
==========================================
  Files          55       55              
  Lines        2522     2548      +26     
==========================================
+ Hits         2295     2321      +26     
  Misses        227      227              
Flag Coverage Δ
#unittests 91.09% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/server/exceptions.py 100.00% <100.00%> (ø)
optimade/server/main.py 95.83% <100.00%> (+0.08%) ⬆️
optimade/server/main_index.py 95.83% <100.00%> (+0.08%) ⬆️
optimade/server/middleware.py 100.00% <100.00%> (ø)

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 c0a1857...929cd38. Read the comment docs.

@CasperWA
Copy link
Member Author

Don't know why the tests are failing here. Locally they run fine for me.

Welp. It seems that maybe the either the pytest_config change in the most top-level conftest.py was not being run for the original CI step that runs tests using mongomock or some of the tests running prior to the tests in said step changes/fixes some things needed for these tests.
I personally think the first one is the case.
If we want to run the original step, I suggest to move to this pytest_config being a pytest fixture instead on the session scope with autouse=True that basically sets the OPTIMADE_CONFIG_FILE environment value (and resets it after all tests have run using try-finally).
However, I'm not sure we need to go back to the original step. The move to a fixture may be needed in any case?

@CasperWA
Copy link
Member Author

When/if this is to be merged, we need to first update the branch protection to use pytest (3.X) instead of deps_static (3.X). But that should be done immediately before this PR is ready to be merged (if we wish to keep in the change of the job name from deps_static to pytest of course).

@CasperWA CasperWA force-pushed the close_391_533-response-for-wrong-versions branch from cfe7646 to aec6824 Compare July 22, 2020 09:58
@CasperWA
Copy link
Member Author

Factored out all out-of-topic changes in this PR and created another PR for them (#422). This means the CI tests for deps_static might fail when getting to run the mongomock pytests.

@CasperWA
Copy link
Member Author

When/if this is to be merged, we need to first update the branch protection to use pytest (3.X) instead of deps_static (3.X). But that should be done immediately before this PR is ready to be merged (if we wish to keep in the change of the job name from deps_static to pytest of course).

This is now not the case for this PR, but rather for #422.

optimade_config.json Outdated Show resolved Hide resolved
@CasperWA CasperWA force-pushed the close_391_533-response-for-wrong-versions branch 2 times, most recently from 31ae83b to 5bc2491 Compare July 22, 2020 17:07
@CasperWA CasperWA mentioned this pull request Jul 22, 2020
1 task
@CasperWA CasperWA force-pushed the close_391_533-response-for-wrong-versions branch from 5bc2491 to dd4a51b Compare July 22, 2020 18:04
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few bits and bobs, mostly to do with testing slashed base URLs without endpoints, otherwise this lgtm

optimade/server/middleware.py Outdated Show resolved Hide resolved
tests/server/test_middleware.py Outdated Show resolved Hide resolved
CasperWA and others added 10 commits July 22, 2020 23:00
The middleware checks the request URL for the versioned part of the URL
path.
This is done in the hopes that the `get_base_url` function in
`optimade.server.routers.utils` returns the correct part of the URL
prior to the versioned part of the URL.
Also, update test_config.json to equal the TestClient.
Also fix version setting in OptimadeTestClient
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
@CasperWA CasperWA force-pushed the close_391_533-response-for-wrong-versions branch from 156127b to 929cd38 Compare July 22, 2020 21:01
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@CasperWA CasperWA merged commit 15a5f7a into Materials-Consortia:master Jul 22, 2020
@CasperWA CasperWA deleted the close_391_533-response-for-wrong-versions branch July 22, 2020 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return 553 for wrongly versioned base URLs
2 participants