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

Improve handling of empty optional params #1332

Closed
wants to merge 1 commit into from

Conversation

pkoenig10
Copy link
Member

Fixes #1331.

Before this PR

When deserializing OptionalInt, OptionalLong, or OptionalDouble values used as header or query parameters, empty string values fail to deserialize with NumberFormatException.

After this PR

When deserializing OptionalInt, OptionalLong, or OptionalDouble values used as header or query parameters, empty string values deserialize as empty optional values.

This is necessary to properly support optional header/query params for Conjure clients due to #790.

This also make the handling of primitive optional consistent with Jersey's handling of Optional<T>:
https://github.com/jersey/jersey/blob/2.25.1/core-server/src/main/java/org/glassfish/jersey/server/internal/inject/ParamConverters.java#L81-L90

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Nov 20, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Improve handling of empty optional params

Fixes #1331.

Before this PR

When deserializing OptionalInt, OptionalLong, or OptionalDouble values used as header or query parameters, empty string values fail to deserialize with NumberFormatException.

After this PR

When deserializing OptionalInt, OptionalLong, or OptionalDouble values used as header or query parameters, empty string values deserialize as empty optional values.

This is necessary to properly support optional header/query params for Conjure clients due to #790.

This also make the handling of primitive optional consistent with Jersey's handling of Optional<T>:
https://github.com/jersey/jersey/blob/2.25.1/core-server/src/main/java/org/glassfish/jersey/server/internal/inject/ParamConverters.java#L81-L90

Possible downsides?

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from markelliot November 20, 2019 22:19
@stale
Copy link

stale bot commented Dec 4, 2019

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Dec 4, 2019
@pkoenig10
Copy link
Member Author

#1337 is the better fix.

@pkoenig10 pkoenig10 closed this Dec 4, 2019
@pkoenig10 pkoenig10 deleted the pkoenig10/optionalParam branch December 4, 2019 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Servers cannot deserialize empty primitive optional values sent by Feign clients
1 participant