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

Reflect CONFIG.root_path in get_base_url #663

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

markus1978
Copy link
Contributor

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
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #663 (9fde7a9) into master (0675ebb) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 9fde7a9 differs from pull request most recent head 11cc921. Consider uploading reports for the commit 11cc921 to get more accurate results
Impacted file tree graph

@@            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%) ⬆️
validator 63.66% <100.00%> (-29.06%) ⬇️

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%) ⬆️
optimade/server/exceptions.py 94.11% <0.00%> (-5.89%) ⬇️
optimade/server/data/__init__.py 70.00% <0.00%> (-5.00%) ⬇️
optimade/server/config.py 88.23% <0.00%> (-2.98%) ⬇️
optimade/server/exception_handlers.py 83.09% <0.00%> (-1.41%) ⬇️
optimade/server/entry_collections/mongo.py 95.31% <0.00%> (-0.85%) ⬇️
optimade/server/main_index.py 94.44% <0.00%> (-0.30%) ⬇️
optimade/server/mappers/entries.py 98.33% <0.00%> (-0.20%) ⬇️
optimade/server/main.py 94.23% <0.00%> (-0.11%) ⬇️
optimade/validator/validator.py 82.31% <0.00%> (-0.05%) ⬇️
... 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.

@markus1978 markus1978 force-pushed the get-base-url-with-root-path branch from c1837bd to 72088b9 Compare January 6, 2021 13:05
@ml-evs
Copy link
Member

ml-evs commented Jan 6, 2021

This sounds related to #520?

@markus1978
Copy link
Contributor Author

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

@ml-evs ml-evs requested review from CasperWA and ml-evs January 7, 2021 14:33
@CasperWA
Copy link
Member

CasperWA commented Mar 3, 2021

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) :)

@CasperWA CasperWA force-pushed the get-base-url-with-root-path branch from 72088b9 to 9fde7a9 Compare March 8, 2021 19:25
@CasperWA CasperWA force-pushed the get-base-url-with-root-path branch from 9fde7a9 to 1235326 Compare March 9, 2021 10:46
@CasperWA
Copy link
Member

CasperWA commented Mar 9, 2021

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.

@CasperWA
Copy link
Member

CasperWA commented Mar 9, 2021

@markus1978 is the expected behaviour that the root_path is added twice to the URL path as seen here?

@ml-evs
Copy link
Member

ml-evs commented Mar 9, 2021

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.

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)

@markus1978 markus1978 requested a review from JPBergsma as a code owner June 1, 2021 14:41
Comment on lines +280 to +283
while value.endswith("/"):
value = value[:-1]
while value.startswith("/"):
value = value[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while value.endswith("/"):
value = value[:-1]
while value.startswith("/"):
value = value[1:]
value = value.strip("/")

I think it would be better to use strip here as it is shorter.

Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

Apart from the comment in config.py, your code looks good to me.
I have, however, just started to work on the Optimade project and there are still plenty of things in the code that I do not know about. So I think it would be best if you wait until you have also received feedback from Matthew or Casper.

@ml-evs ml-evs added the on-hold For PRs/issues that are on-hold for an unspecified time label Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-hold For PRs/issues that are on-hold for an unspecified time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants