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

#92 url parameters meegeven #93

Merged
merged 2 commits into from
Mar 28, 2023
Merged

Conversation

vancamti
Copy link
Contributor

@vancamti vancamti commented Mar 27, 2023

Dit werkt in de simpele test case, maar is dit robuust/veilig genoeg ?

@coveralls
Copy link

coveralls commented Mar 27, 2023

Coverage Status

Coverage: 100.0%. Remained the same when pulling db640ce on feature/92_url_params_meegeven into 4183dde on develop.

@@ -37,7 +37,8 @@ def handle(self, uri, request):
redirect = self._get_redirect_based_on_accept_header(
request.accept, redirect
)
redirect = redirect.format(**m.groupdict())
unmatched_part = (uri.replace(m.group(), ""))
redirect = redirect.format(**m.groupdict()) + unmatched_part
Copy link
Contributor

@Wim-De-Clercq Wim-De-Clercq Mar 27, 2023

Choose a reason for hiding this comment

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

Ik zou gwn request.query_string gebruiken ipv unmatched part. Het ? zit er wel niet bij.

Copy link
Contributor Author

@vancamti vancamti Mar 27, 2023

Choose a reason for hiding this comment

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

dat is leeg bij mij, de query paramaters zijn onderdeel van de bevraagde uri, geen echte query paramaters
nvm verkeerde test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maar die query params zijn wel degelijk onderdeel vd uri:
image
ik zou er wel een substring uit kunnen nemen ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh right, by die andere APIs wel ja. Ik had enkel de redirect route getest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

denk dat die params wel altijd best url encoded doorkomen dat het als onderdeel wordt gezien vd uri anders werkt het niet

Copy link
Contributor

Choose a reason for hiding this comment

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

In de praktijk zijn alle urls wel gedefinieert met $ achteraan. Dus de match zal falen daarmee.
https://github.com/OnroerendErfgoed/scripts/blob/master/servers/joeri/config/urihandler.yaml

De queryparams zullen eraf moeten voor de match. Ik denk dat simpele string manipulatie wel ok is. Gewoon alles na het eerste vraagteken eraf, en bijhouden om er later terug aan te plakken.

@vancamti vancamti force-pushed the feature/92_url_params_meegeven branch from 0bfbac8 to 743e02e Compare March 28, 2023 07:57
@vancamti vancamti merged commit 9a58044 into develop Mar 28, 2023
@vancamti vancamti deleted the feature/92_url_params_meegeven branch March 28, 2023 09:05
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.

3 participants