optimade-python-tools
optimade-python-tools copied to clipboard
Reflect CONFIG.root_path in get_base_url
The current implementation assumes no API path prefix (e.g.CONFIG.root_path==None
). If there is a prefix some URLs are wrong.
The fix in this PR is on get_base_url
and also on one use of get_base_url
. The usages of get_base_url
assume different things. For the info endpoint the expectation is that get_base_url
returns a base url including the path prefix (untouched). For the next links in entry list endpoints, the expectation is that get_base_url
returns the base url with no path at all and it will add the request url's path including the path prefix (fixed here). You could also fix it the other way around.
Codecov Report
Merging #663 (9fde7a9) into master (0675ebb) will increase coverage by
0.07%
. The diff coverage is100.00%
.
:exclamation: Current head 9fde7a9 differs from pull request most recent head 11cc921. Consider uploading reports for the commit 11cc921 to get more accurate results
@@ Coverage Diff @@
## master #663 +/- ##
==========================================
+ Coverage 92.72% 92.80% +0.07%
==========================================
Files 68 67 -1
Lines 3668 3528 -140
==========================================
- Hits 3401 3274 -127
+ Misses 267 254 -13
Flag | Coverage Δ | |
---|---|---|
project | 92.80% <100.00%> (+0.07%) |
:arrow_up: |
validator | 63.66% <100.00%> (-29.06%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
optimade/server/routers/utils.py | 98.55% <100.00%> (+0.02%) |
:arrow_up: |
optimade/server/exceptions.py | 94.11% <0.00%> (-5.89%) |
:arrow_down: |
optimade/server/data/__init__.py | 70.00% <0.00%> (-5.00%) |
:arrow_down: |
optimade/server/config.py | 88.23% <0.00%> (-2.98%) |
:arrow_down: |
optimade/server/exception_handlers.py | 83.09% <0.00%> (-1.41%) |
:arrow_down: |
optimade/server/entry_collections/mongo.py | 95.31% <0.00%> (-0.85%) |
:arrow_down: |
optimade/server/main_index.py | 94.44% <0.00%> (-0.30%) |
:arrow_down: |
optimade/server/mappers/entries.py | 98.33% <0.00%> (-0.20%) |
:arrow_down: |
optimade/server/main.py | 94.23% <0.00%> (-0.11%) |
:arrow_down: |
optimade/validator/validator.py | 82.31% <0.00%> (-0.05%) |
:arrow_down: |
... and 6 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 0675ebb...11cc921. Read the comment docs.
This sounds related to #520?
I am not sure. I think it is different. #520 is about a proxied app that gets a different request url from the proxy than from "outside". This PR wont fix that. I think this might be cause by wrong proxy config? If you forward the host correctly, the request urls should match the actual "outside" request urls. It least we did not run into issues running this in docker behind a proxy.
But it is definitely related to #634
I am not sure. I think it is different. #520 is about a proxied app that gets a different request url from the proxy than from "outside". This PR wont fix that. I think this might be cause by wrong proxy config? If you forward the host correctly, the request urls should match the actual "outside" request urls. It least we did not run into issues running this in docker behind a proxy.
There isn't any issues with the added routers either, only the documentation routes added by the FastAPI itself (just a note) :)
I've added a root_path
to the test configuration file that's loaded for pytests. I think this uncovers an issue with the implementation, specifically concerning the middleware. This might be similar to the issue described in #520 - it might not.
Also, I've added a fix to one issue I found locally already, namely that if trailing slashes (/
) are used for the root_path
, then it was used wrongly.
It might be worth making sure that there's a single starting slash as well? Edit: I have added this now in 863c0bb along with a "test" of it, by having it as the configuration value in the test config file.
@markus1978 is the expected behaviour that the root_path
is added twice to the URL path as seen here?
Also, I've added a fix to one issue I found locally already, namely that if trailing slashes (
/
) are used for theroot_path
, then it was used wrongly.
Not sure if this is related, but I recently noticed that http://localhost:5000/v1 returns with a wrong version error on the reference server now (i.e. it doesn't redirect to the landing page)