Implement Guava Range deserialization from bracket notation (#135)

[mukham12]

Initiated implementation of a feature specified by #135 for deserializing bracket notation Strings into Guava Range objects. Refactored tests to eliminate repetition and introduced a new method for deserialization testing, expanding the range of test cases.

While substantial progress has been made, there’s still work to be done. The current implementation primarily supports integer deserialization (infinities are taken into account).

However, handling of Duration, LocalDate, and other objects is pending due to the unfamiliarity of deserialization for these types. Looking forward to guidance from @cowtowncoder on these aspects.

Despite these shortcomings, pleased with the progress made so far. 🚀

[cowtowncoder]

Ok, added some notes on improving validation, error handling, but in general looks good.

But I do wonder about support for other types; while deserialization delegation works well in general, that won't really work in this case since it uses sort of "embedded" Strings, as opposed to JSON constructs. That is, there is no in-built support for this style of textual structuring. Does not mean it can't be done, just that reuse of existing functionality probably needs refactoring, and cannot use JsonDeserializer delegation. But maybe addition of shared static helper methods within other deserializers.

[mukham12]

Thank you for your guidance. I will address the points you’ve mentioned at the earliest opportunity. The thought of validation and exception handling did cross my mind, but only after the PR was created. I’m glad you brought it to my attention regardless.

Additionally, I’ve been intending to inquire, would there be a need for me to modify the deserializeWithType method?

I’m still in the process of understanding many of the concepts/terminology used within the project, so please forgive me if my questions seem naive.

[cowtowncoder]

Thank you for your guidance. I will address the points you’ve mentioned at the earliest opportunity. The thought of validation and exception handling did cross my mind, but only after the PR was created. I’m glad you brought it to my attention regardless.

Additionally, I’ve been intending to inquire, would there be a need for me to modify the deserializeWithType method?

Hmmh. I am not 100% sure; due to different shapes, possibly, but let's defer this until there's actual need. That method is needed to support polymorphic deserialization, and this in turn is a feature that does not mix very well with generic types such as Range.

As to "actual need": that could be determined by writing a test case that tries to serialize + deserialize a Range instance with polymorphic handling: something like java.lang.Object valued property with @JsonTypeInfo. I would probably leave that to a later stage but it is up to you.

I’m still in the process of understanding many of the concepts/terminology used within the project, so please forgive me if my questions seem naive.

Not at all, so far all questions perfectly reasonable and valid :)

[mukham12]

Tatu,

I have incorporated your suggestions and also included a test to verify the exception in case of an invalid bracket range. Please feel free to inform me if there are any further additions or removals required.

In the meantime, I will continue to explore this further and assess the effort required to implement deserialization for generic objects.

[mukham12]

Just a friendly reminder about this, @cowtowncoder. If any issues are preventing you from merging, please let me know how I can be of help!

[cowtowncoder]

@mukham12 No, the limiting thing is my time unfortunately. But now that I am here, I'll have a look -- I think I have some minor suggestions, and then we are good to go.

[cowtowncoder]

Ok, so did make same cosmetic changes. Things look mostly ok.

Except. I now realized the real problem which I did not fully appreciate earlier.

The whole Range serialization as String is assuming that endpoint types' .toString() method is used for serialization. That is not really good and I am not sure why I didn't pick on it earlier. This is different from "full Object" serialization, for which RangeSerializer at least uses generic type to find actual value serializer. Similarly RangeDeserializer does fetch proper deserializer.

But for String style it really isn't. It can be cobbled together to work for some types. Code as-is will fail miserably for most types beyond simple numbers.

Jackson almost has something applicable since there Key Serializers (JsonSerializer implementation, but one that writes JsonToken.FIELD_NAME) and Key Deserializers (separate KeyDeserializer type). They deal with JSON Strings, basically, but unfortunately require read from JsonParser and write using JsonGenerator, reading/writing field name. What we'd need here instead would be something to convert between String and (de)serialized type.

But, at least code in RangeDeserializer.createContextual() accesses endpoint type. So code being added will need to make use of that to figure out handling -- instead of heuristically trying to deduce wrong type.

And as importantly, I think, should explicitly fail for unsupported types.

So I think what I'd like to see is code that does some of that. But even with basic numbers it gets hairy; there are so many Number types.

But let me think about this a bit more....

[cowtowncoder]

Whoa! I should have looked at KeyDeserializer API first before claiming it won't work. In fact, it would seem to work:

public abstract class KeyDeserializer
{
    public abstract Object deserializeKey(String key, DeserializationContext ctxt)  throws IOException;

So it CAN be passed String and not JsonParser.

I think this can and should be used: call from createContextual() to fetch KeyDeserializer (only in case of Shape.STRING):

ctxt.findKeyDeserializer(_rangeType, property)

and error out if none found. Cool thing is that this should work for a few types and (more importantly) can be extended by modules to support all kinds of 3rd party types.

Same isn't true for Key Serializers as they just implement JsonSerializer. But this might be good enough for now.