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

Fix toQueryString function in UrlUtil for null values #357

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

chrismayer
Copy link
Collaborator

This PR fixes the toQueryString function in the UrlUtil in case a null value is passed in for a parameter. Main reason is that typeof null resolves to 'object' (thanks JavaScript 🙄 ), which leads to a recursion with null instead of an object.

@fschmenger
Copy link
Collaborator

Hi Christian, looks good to me. One question here: Is it required to encode params as null or should they just be removed from the query string?
If the answer is 'yes' then feel free to merge.

@chrismayer chrismayer force-pushed the fix-null-toQueryString branch 2 times, most recently from 884ab9b to aeb81ee Compare November 13, 2023 09:41
@chrismayer
Copy link
Collaborator Author

Thanks for your review @fschmenger. I don't think it is required to encode URL params as null and maybe a bad idea if I think twice. There might be typing problems in some API implementations receiving foo=null, since null is then a string and might not match a defined data type.

So I decided to rework this a bit and remove null values from the URL query string. Would you please give this another quick check, @fschmenger ? If you are OK with the code please hit the Approve button, otherwise the PR is not mergable... Thanks in advance.

@fschmenger
Copy link
Collaborator

Ok, looks good.

@chrismayer
Copy link
Collaborator Author

Thank you @fschmenger 🙏 Going to merge now...

@chrismayer chrismayer merged commit 8571561 into wegue-oss:master Nov 13, 2023
1 check passed
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.

2 participants