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

Return null instead of throwing exception when querystring parameter is missing #5326

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

rutterpaul-personal
Copy link
Contributor

See #5260

Instead of throwing IllegalArgumentException when querystring parameter value is null (e.g. not present), just return null.
Throwing exceptions is expensive, especially when it's done for every querystring parameter that has no @DefaultValue (and is not present in the request querystring).

I'm still looking for a good place to add a unit test. Would ParamConverterInternalTest be it?

@rutterpaul-personal
Copy link
Contributor Author

@jansupol i tried to supply a new PR under my personal account, where i signed the ECA.
But it doesn't seem to accept it, although the github account is properly matching the one of this PR.
What did i miss?

Is it because of the commit message not being in the format defined in https://www.eclipse.org/projects/handbook/#resources-source?

@paulrutter
Copy link

paulrutter commented May 12, 2023

At least the build succeeded and the ECA is ok.
What else do we need to get this into a release?

Any tips on where i would add a unit test?

@jansupol
Copy link
Contributor

jansupol commented Jun 6, 2023

Sorry for the delay, still planning this for review for 2.40...

@jansupol
Copy link
Contributor

jansupol commented Jun 8, 2023

The test...it depends, it could be in ParamConverterInternalTest, or for end-to-end tests in ParamConverterTest.

@jansupol jansupol merged commit 08a85c7 into eclipse-ee4j:master Jun 15, 2023
@senivam senivam added this to the 2.40 milestone Jun 20, 2023
This was referenced Jun 23, 2023
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.

4 participants