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

Handle edge cases with NaN/Infinity #1624

Merged
merged 5 commits into from
Dec 5, 2024
Merged

Handle edge cases with NaN/Infinity #1624

merged 5 commits into from
Dec 5, 2024

Conversation

lewisjkl
Copy link
Contributor

@lewisjkl lewisjkl commented Dec 4, 2024

When using a Query Parameter of type Double that has a Range constraint, NaN values will cause an exception to be thrown that would lead to a 500 error being returned by the service. The reason for this is that the Range RefinementProvider uses BigDecimal under the hood and BigDecimal does not allow NaN.

This PR updates the Range RefinementProvider to return an error that will map to a 4xx response. Additionally, the MetadataDecoder has been updated to allow configuring whether or not NaN should be allowed. AWS protocols allow NaN, whereas alloy#simpleRestJson does not.

Tests have been added to show that NaN is not currently supported by the json module or the Document type.

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

Will add relevant docs and update changelog once we know this is the direction we want to go.

@lewisjkl lewisjkl requested a review from Baccata December 4, 2024 23:28
@Baccata Baccata merged commit 2522c02 into series/0.18 Dec 5, 2024
2 checks passed
@Baccata Baccata deleted the nan-edge-cases branch December 5, 2024 16:44
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