Workaround for URL encoding in pydantic 1.10

[JPBergsma]

I am trying to solve the issues in PR #1316 with this PR, so this PR can be merged instead of PR #1316.

[codecov]

Codecov Report

Merging #1317 (32e2f91) into master (fdb948d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1317   +/-   ##
=======================================
  Coverage   90.90%   90.90%           
=======================================
  Files          72       72           
  Lines        4364     4366    +2     
=======================================
+ Hits         3967     3969    +2     
  Misses        397      397           

Flag	Coverage Δ
project	90.90% <100.00%> (+<0.01%)	⬆️
validator	90.90% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
optimade/server/routers/utils.py	98.16% <100.00%> (+0.03%)	⬆️
optimade/validator/validator.py	83.54% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ml-evs]

I'm still not entirely sure what is causing this bug to trigger in these dependency updates. Could you perhaps make a separate PR that just includes this test, so we can see if it fails? I'd also recommend just sticking it at the end of tests.servers.routers.test_structures rather than in the test_utils.py file.

[JPBergsma]

The bug is triggered by pydantic 1.10. Basically, pydantic now also encodes the query part of the url.
That was however already done in our code with urllib.parse.encode. So we have to turn of one of the two.

See more info at https://github.com/pydantic/pydantic/issues/3061and pydantic/pydantic#4224
And this bug report: pydantic/pydantic#4458
It looks like they are already working on a fix : pydantic/pydantic#4461
Although there is still some discussion about whether it should be implemented.

There are also still some problems with spaces. Not all browsers can handle a + sign correctly. So my solution may not work for everybody. I also tried to keep the spaces, but then I get an error:

status	"500"
code	"value_error.url.extra"
title	"ValidationError"
detail	"URL invalid, extra characters found after valid URL: ' HAS "Ac"&page_limit=2&page_offset=2'"
source
pointer	"/next"

So maybe we should wait to see if it gets accepted.

This test mostly checks whether the next link is in a format that will be handled correctly by pydantic 1.10.
It does not test whether the next link is valid. For that, we should use _test_page_limit from validator.py

I deliberately put it in test_utils.py as it applies to all endpoints and not just structures as the affected code is also in utils.py. I can use the EntryResponseMany class instead of the StructureResponseMany class if you prefer.

[ml-evs]

Yeah, let's wait. It looks like the main author is working on a fix (as of 30 mins ago).

[ml-evs]

I still think it would be useful to add the test to test_structures.py in a separate PR though.

[ml-evs]

Suggested change

	urlencoded = urllib.parse.urlencode(
	query, doseq=True, quote_via=dummy_quote
	) # it seems the url is also quoted at a later stage so we should not quote it here, hence the dummy_quote.
	urlencoded = urllib.parse.urlencode(
	query, doseq=True, quote_via=lambda x, *_: x.replace(" ", "+")
	)

is probably a bit cleaner. I had a look at the built-in urllib.parse.quote method, not sure how it differs from this when passed to quote_via.

[JPBergsma]

Your suggestion is indeed cleaner. I'll use a lambda function instead.

[ml-evs]

Now that pydantic 1.10.2 is out and #1316 is passing, I think this can be closed.