-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Allow serialization of InetAddress
as simple numeric host address
#1605
Comments
Thank you reporting this. I agree in that handling is sub-optimal, but I am not quite sure what to do about that at this point. I do not really look forward to having to add elaborate handling. |
What level of backwards compatibility is required for a minor version like 2.9? What about 3.0? If we can agree on a strategy, I’d be happy to prepare a pull request. A simple improvement would be to always serialize to the address—never the hostname. This would be a net reduction in code and would eliminate the possibility of network access upon deserialization for JSON produced using this library, at least. If hostname preservation is important, we could serialize to the |
@2is10 I assume that the critical think would be to support deserialization from both textual and numeric representations. Serialization compatibility is probably little bit less critical. If you are interesting in tackling this, I'd be happy to help -- I am bit overloaded with issues in general, but can help with reviewing and getting things merged. This change could still make it in 2.9: I am planning to do |
Actually I think that maybe leaving default serialization unchanged makes sense for 2.9. So: what I will do is to support use of ObjectMapper mapper = new ObjectMapper();
mapper.configOverride(InetAddress.class)
.setFormat(JsonFormat.Value.forShape(JsonFormat.Shape.NUMBER)); to allow changing output to use host address when "use number" is indicated. And for 3.0 we can and should change the default (whether to allow textual is an open question; but amount of code for that is rather small so not a big deal. |
InetAddress
as simple numeric host address
Thanks! No changes to default behavior on a minor version release seems like a reasonable policy. It could be a little surprising that Here’s some brainstorming for 3.0 formats. Depending on whether the hostname is known:
I suppose Notes:
|
@2is10 I agree that such choice for "number" is bit too clever, but given that Interesting wrt methods that can avoid lookup. Maybe something based on those could still be used for deserialization, even for 2.9? |
Deserialization currently uses In 2.9, if |
@2is10 Thank you for thorough explanation: makes sense, good to know there are heuristics so things will work sort of acceptably for now. |
An
InetAddress
is defined this way in its Javadoc:The IP address is central. The hostname is optional, peripheral.
However, Jackson’s InetAddressSerializer just uses the hostname, if present, and discards the IP address. This is critical information loss. A hostname isn’t guaranteed to be resolvable to the same IP address (or at all) where/when deserialized.
A corresponding issue with the deserializer is that hostname resolution (network access) is attempted for any input string that isn’t a valid IP address. This violates the Principle of Least Astonishment.
These seem like important issues to fix, but I’m not sure how to do it in a backwards-compatible way. The JDK’s
InetAddress
is partly to blame, the minefield that it is.I suggest taking inspiration from Guava’s InetAddresses and doing some of your own parsing.
You’ll need to decide whether to serialize to just the IP address or to try to also preserve the hostname, if present. For what it’s worth, my team’s present use cases only care about the IP address. However, any information loss could be surprising to people who use this library.
The text was updated successfully, but these errors were encountered: