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

Add support for string amounts #42

Merged

Conversation

bountin
Copy link
Contributor

@bountin bountin commented May 2, 2017

I'm consuming an API that serializes the amounts as string values and the current implementation fails at deserialization with a JsonMappingException (Current token (VALUE_STRING) not numeric, can not use numeric value accessors)

I thought it doesn't hurt to support this use case in the library and so here is my PR :)

The way via the context should not add much overhead performance wise as the BigIntegerDeserializer does more or less the same in the INT and FLOAT cases but also handles Strings correctly. The indirection just adds some more stack frames.

@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented May 2, 2017

@whiskeysierra

  1. Should it be liberal to deserialize both types or there should be a deserialization feature enabled to support Strings? I would propose to act as Jackson core acts to follow least astonishment principle.
  2. I think there should be an implementation+test and feature switch to serialize into String as well.

@whiskeysierra
Copy link
Collaborator

@bountin Thanks for the contribution!

Should it be liberal to deserialize both types or there should be a deserialization feature enabled to support Strings?

Jackson does it without configuration, by default? In that case I'd agree to do the same.

I think there should be an implementation+test and feature switch to serialize into String as well.

With Postel's Law in mind:

Be conservative in what you send, be liberal in what you accept.

I'd prefer not to support it, since it violates our schema for monetary values.

@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented May 2, 2017

@whiskeysierra

I'd prefer not to support it, since it violates our schema for monetary values.

If there is a system from which client reads Strings, I am sure there is (or will be) a system which will require Strings as input

@whiskeysierra
Copy link
Collaborator

I am sure there is (or will be) a system which will require Strings as input

That system will violate both, our schema as well as Postel's law. Let's wait for that to happen and add support for it, if and when needed.

@whiskeysierra
Copy link
Collaborator

👍

@whiskeysierra whiskeysierra merged commit 22fa76d into zalando:master May 2, 2017
@whiskeysierra
Copy link
Collaborator

@bountin I released 0.12.0. Should be in central, soon.

@bountin
Copy link
Contributor Author

bountin commented May 3, 2017

Thanks for the fast merge & release 👍

@bountin bountin deleted the deserialize-amount-from-string branch May 3, 2017 11:46
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