Add IP address field type.

[mgetka]

This pull request adds IPv4/IPv6 fields serialization and deserialization w/ tests included. To avoid confusion I am excluding the possibility to specify IP addresses as an integer or byte values.

[lafrech]

Should we add a parameter to only accept IPv4 or IPv6 rather than accepting any?

Should we also provide IPv4 and IPv6 fields as a shortcut?

Also, I don't see the point of the _serialize method. Apparently you copied it from an already existing field (UUID) so it's nothing new. But this seems a bit dodgy to me. Let's discuss this in #1489.

[mgetka]

Also, I don't see the point of the _serialize method. Apparently you copied it from an already existing field (UUID) so it's nothing new. But this seems a bit dodgy to me.

That is correct, I've overridden the _serialize based on insights collected from UUID field implementation. After inspecting the implementation of String base class, I can no longer see any purpose of _serialize method.

[mgetka]

Also please note that exploded notation is specific for IPv6 addresses, but the argument is also present in IPv4 constructor. This feature is sourced from ipaddress python standard library, where exploded notation can be applied to both IPv4 and IPv6 for consistency.

[sloria]

Would it make sense to implement these as validators? That would allow combining with other validators.

[mgetka]

I have to admit that maybe I was too inspired by the implementation of the UUID field, and implemented the functionality without more complete understanding of fields module structure. As I can see, fields inheriting from String, are in general deserialized from strings to strings with additional input value format validation. We can see it in the Url and Email fields cases. Exception from this rule is the UUID field, which deserializes from string into UUID object representation. Therefore, it is not clear for me why UUID inherits from String and not Field, like in the case of DateTime or TimeDelta which also deserializes to object representation.

ipaddress library utilities provides with IP address object representations with validation occurring on object construction. In my opinion creation of the object representation for the purpose of validation, and then discarding it would be an overkill. Also I can see clear contract between validators based implementation of Url and Email fields and String base class features. But IP addresses are not the case - they are more like DateTime or TimeDelta fields. Therefore, IP field now inherits from Field and not String.

Also, I can see, that _validated method is utilized for chained validation in inheriting fields classes.
For IP fields it is not the case, since IPv4 and IPv6 Fields implements its own independent validation procedures. I have moved validation/object creation into _deserialize methods.

Returning to the case of the UUID field. Maybe it should be discussed whether it should inherit from String or rather Field to get a more transparent structure of the module?

I am also wondering whether IP specific validators are welcome? Based on ipaddress functionalities it is straightforward to implement validators like: PrivateIPAddress, GlobalIPAddress or one that checks whether IP address belongs to specified subnetwork.

[lafrech]

Looks good to me.

Just a question/comment about a test. I wouldn't mind another opinion here.

Your remarks about UUID field make sense. Reworking it could be done in another PR, and perhaps in marshmallow 4 if it is a breaking change.

IP specific validation welcome. Should those be field constructor arguments instead like exploded?

[sloria]

@lafrech @deckar01 Can one of you do a final CR pass on this? Looks like it's close to--if not completely--ready to merge.

[lafrech]

LGTM. I'm just concerned about the code duplication. Could _deserialize be factorized, using class attributes for the object class, like what is done for the error message? (The typing would be a little less strict, but oh well.)

[mgetka]

LGTM. I'm just concerned about the code duplication. Could _deserialize be factorized, using class attributes for the object class, like what is done for the error message? (The typing would be a little less strict, but oh well.)

I've tried to define class variable typed as typing.Callable[[str], typing.Union[ipaddress.IPv4Address, ipaddress.IPv6Address]] which holds ipaddress.ip_address for generic IP field, and ipaddress.IPv4Address and ipaddress.IPv6Address classes for IPv4 and IPv6 fields respectively, but there are issues with ipaddress.ip_address becoming a method. The issue can be suppressed by decorating the ipaddress.ip_address with staticmethod like

DESERIALIZATION_CALLABLE = staticmethod(ipaddress.ip_address)

but I'm not entirely comfortable with such construct. Also, mypy doesn't like it as well since there is unresolved issue #3482 which is of relevance for class methods and static methods defined this way.

[lafrech]

Sorry for the delay and thanks for your work on this.

[lafrech]

@sloria, @deckar01, any objection to merging this?.

[sloria]

@lafrech I didn't to a fine-toothed review of this, but it looked good last time I looked. I defer to you on merging this.

[lafrech]

Merged and released.

Thanks again @mgetka.