Add `api_hint` query parameter #421

CasperWA · 2020-07-21T19:31:16Z

Closes #392

This parameter is handled in a Middleware.
This is because it can lead to changing the original request by moving to a versioned base URL.

We also need to include a new Exception, which is actually an HTTP response, similar to BadRequest, namely VersionNotSupported (553).

The addition of the parameter to query_params.py is merely for adding it to the OpenAPI specification.

Missing:

Tests
Add warnings
Add better doc strings

codecov · 2020-07-21T19:48:56Z

Codecov Report

Merging #421 into master will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
+ Coverage   91.45%   91.60%   +0.14%     
==========================================
  Files          60       60              
  Lines        2751     2800      +49     
==========================================
+ Hits         2516     2565      +49     
  Misses        235      235

Flag	Coverage Δ
#unittests	`91.60% <100.00%> (+0.14%)`	⬆️

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

Impacted Files	Coverage Δ
optimade/server/query_params.py	`100.00% <ø> (ø)`
...made/server/entry_collections/entry_collections.py	`98.82% <100.00%> (ø)`
optimade/server/main.py	`96.07% <100.00%> (+0.07%)`	⬆️
optimade/server/main_index.py	`96.07% <100.00%> (+0.07%)`	⬆️
optimade/server/middleware.py	`94.93% <100.00%> (+2.07%)`	⬆️
optimade/server/routers/utils.py	`91.15% <100.00%> (+0.06%)`	⬆️
optimade/server/warnings.py	`85.71% <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 c4e6f3f...11fbb9b. Read the comment docs.

CasperWA · 2020-07-24T11:53:21Z

So I've been slightly rewriting according to some misinterpretation on my part, I think.
I understood the api_hint as a form of redirect, but I think this is not the case.
Instead it should simply serve the response as best it can without redirecting, but it may do so underneath the hood though, however, a proper redirect / forwarded response should never be returned, i.e., no 30X responses.

How do you understand this?

The issues I was having was that I was using a RedirectResponse to redirect to the versioned base URLs, but this would then resolve in headers not being forwarded as well. And now I've been trying to muck about trying to get that work and it dawned on me that I don't need to actually have a RedirectResponse...

CasperWA · 2020-07-24T12:51:23Z

So I've been slightly rewriting according to some misinterpretation on my part, I think.
I understood the api_hint as a form of redirect, but I think this is not the case.
Instead it should simply serve the response as best it can without redirecting, but it may do so underneath the hood though, however, a proper redirect / forwarded response should never be returned, i.e., no 30X responses.

How do you understand this?

The issues I was having was that I was using a RedirectResponse to redirect to the versioned base URLs, but this would then resolve in headers not being forwarded as well. And now I've been trying to muck about trying to get that work and it dawned on me that I don't need to actually have a RedirectResponse...

I've kept in both solutions for now.
I also just realized that sending headers in a request does of course not also mean that one should get those headers in the response ...

Currently, I've implemented the "cleaner" solution of using a RedirectResponse to visibly show the redirect.

shyamd

Two lazy-loaded imports I have an issue with, but otherwise, this looks good.

optimade/server/middleware.py

CasperWA · 2020-07-31T14:47:12Z

@shyamd I have moved all imports to the top-level of the middleware file now. It passed all tests locally. Let's see what happens here now with the CI.

This parameter is handled in a Middleware. This is because it can lead to changing the original request by moving to a versioned base URL. We also need to include a new Exception, which is actually an HTTP response, similar to BadRequest, namely VersionNotSupported (553). The addition of the parameter to `query_params.py` is merely for adding it to the OpenAPI specification.

Fix HandleApiHint according to mistakes found from tests.

Also, updated a Warning name used in sort parameter. And fix attributes in collections by moving them from MongoCollection to EntryCollection.

Move `AddWarning` middleware to be the last added middleware. This is to ensure it becomes the first custom middleware invoked, in order to override the `warnings.showwarning` function.

This found a mistake in the middleware, where `None` returns from `handle_api_hint()` was not handled. This has been fixed. Also, add extra parsing of values to support `,`-separated list values.

optimade/server/query_params.py

ml-evs

LGTM now! Cheers @CasperWA

* Add `api_hint` query parameter: This parameter is handled in a middleware. This is because it can lead to changing the original request by moving to a versioned base URL. The addition of the parameter to `query_params.py` is merely for adding it to the OpenAPI specification. * Split middleware tests into separate files. * Fix middleware inclusion sequence: Move `AddWarning` middleware to be the last added middleware. This is to ensure it becomes the first custom middleware invoked, in order to override the `warnings.showwarning` function.

CasperWA force-pushed the close_392_api_hint-query-parameter branch 3 times, most recently from 6a999d8 to 68d382f Compare July 22, 2020 23:17

CasperWA requested a review from ml-evs July 24, 2020 12:49

CasperWA marked this pull request as ready for review July 24, 2020 12:49

CasperWA force-pushed the close_392_api_hint-query-parameter branch from 346c0fa to 47f6a16 Compare July 30, 2020 21:35

shyamd previously approved these changes Jul 31, 2020

View reviewed changes

optimade/server/middleware.py Outdated Show resolved Hide resolved

optimade/server/middleware.py Show resolved Hide resolved

CasperWA dismissed shyamd’s stale review via ca76bd3 July 31, 2020 14:46

CasperWA requested a review from shyamd July 31, 2020 14:50

shyamd previously approved these changes Jul 31, 2020

View reviewed changes

CasperWA added 13 commits July 31, 2020 17:24

Split middleware tests into separate files

0be6f45

Fix wrong rebase of CheckWronglyVersioned...

c47c49b

Use HandleApiHint middleware in servers

c190816

Update response pytest fixtures

e989686

Minor else clause addition for completeness

c0368d3

Add tests for HandleApiHint

38f1b99

Fix HandleApiHint according to mistakes found from tests.

Remove api_hint query param after redirect

c6fb934

Use Warnings in api_hint middleware

38169fd

Also, updated a Warning name used in sort parameter. And fix attributes in collections by moving them from MongoCollection to EntryCollection.

Add tests for api_hint warnings

f8f894a

Move `AddWarning` middleware to be the last added middleware. This is to ensure it becomes the first custom middleware invoked, in order to override the `warnings.showwarning` function.

Extra test for warnings in response

c0151ee

This found a mistake in the middleware, where `None` returns from `handle_api_hint()` was not handled. This has been fixed. Also, add extra parsing of values to support `,`-separated list values.

Remove superfluous/unused Warnings

e1bf558

Move all imports to top-level

ee043e2

CasperWA dismissed shyamd’s stale review via ee043e2 July 31, 2020 15:27

CasperWA force-pushed the close_392_api_hint-query-parameter branch from ca76bd3 to ee043e2 Compare July 31, 2020 15:27

ml-evs reviewed Jul 31, 2020

View reviewed changes

optimade/server/query_params.py Show resolved Hide resolved

CasperWA added 3 commits July 31, 2020 17:58

Add documentation

a3a4ce8

Fix minor documentation bugs

651a6c5

Add api_hint to query_params attributes

11fbb9b

CasperWA requested review from shyamd and ml-evs July 31, 2020 16:09

ml-evs approved these changes Jul 31, 2020

View reviewed changes

CasperWA merged commit 033800d into Materials-Consortia:master Jul 31, 2020

CasperWA deleted the close_392_api_hint-query-parameter branch July 31, 2020 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `api_hint` query parameter #421

Add `api_hint` query parameter #421

CasperWA commented Jul 21, 2020 •

edited

Loading

codecov bot commented Jul 21, 2020 •

edited

Loading

CasperWA commented Jul 24, 2020

CasperWA commented Jul 24, 2020

shyamd left a comment

CasperWA commented Jul 31, 2020

ml-evs left a comment

Add api_hint query parameter #421

Add api_hint query parameter #421

Conversation

CasperWA commented Jul 21, 2020 • edited Loading

codecov bot commented Jul 21, 2020 • edited Loading

Codecov Report

CasperWA commented Jul 24, 2020

CasperWA commented Jul 24, 2020

shyamd left a comment

Choose a reason for hiding this comment

CasperWA commented Jul 31, 2020

ml-evs left a comment

Choose a reason for hiding this comment

Add `api_hint` query parameter #421

Add `api_hint` query parameter #421

CasperWA commented Jul 21, 2020 •

edited

Loading

codecov bot commented Jul 21, 2020 •

edited

Loading