Jackson subtypes for ReturnItem are not configured against their respective interfaces

[Akii]

Hi,

I noticed that the subtypes of ReturnItem are configured against the default implementation even though both CustomLineItemReturnItemImpl and LineItemReturnItemImpl have an interface.

@JsonSubTypes({
        @JsonSubTypes.Type(value = com.commercetools.api.models.order.CustomLineItemReturnItemImpl.class, name = CustomLineItemReturnItem.CUSTOM_LINE_ITEM_RETURN_ITEM),
        @JsonSubTypes.Type(value = com.commercetools.api.models.order.LineItemReturnItemImpl.class, name = LineItemReturnItem.LINE_ITEM_RETURN_ITEM) })
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", defaultImpl = ReturnItemImpl.class, visible = true)
public interface ReturnItem

The interfaces CustomLineItemReturnItem and LineItemReturnItem also narrow the type down to their respective default implementation.

@JsonDeserialize(as = CustomLineItemReturnItemImpl.class)
public interface CustomLineItemReturnItem extends ReturnItem

That makes me think that the Jackson configuration of ReturnItem could look like this:

@JsonSubTypes({
        @JsonSubTypes.Type(value = com.commercetools.api.models.order.CustomLineItemReturnItem.class, name = CustomLineItemReturnItem.CUSTOM_LINE_ITEM_RETURN_ITEM),
        @JsonSubTypes.Type(value = com.commercetools.api.models.order.LineItemReturnItem.class, name = LineItemReturnItem.LINE_ITEM_RETURN_ITEM) })
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", defaultImpl = ReturnItemImpl.class, visible = true)
public interface ReturnItem

That way the Jackson configuration would be more streamlined with how the rest of the SDK works. And I think this should not even be a breaking change.

I'm not a Jackson pro so there's a good chance I'm wrong. I marked this as "bug" but please go ahead and label this to what you think is best.

Thank you!

[jenschude]

I think the main reasoning was to avoid an additional layer of indirection.

Polymorphic interface -> subtype interface -> Subtype Impl

And as long as there is no specific reason I would not touch it. Especially as through mixins it's still possible to override the annotations

[Akii]

I think the main reasoning was to avoid an additional layer of indirection.

That sounds a lot like premature optimization. There is a lot of indirection in the library already imo, and this would be "more consistent".

The specific reason is that 2 mixins are required if you'd want to replace the implementation of either of the two subclasses.

To be fair, I don't know if there is an actual use-case since I'm not familiar with the underlaying business cases. I'm just looking at the technicals here, and the general look-and-feel of the SDK.

Then again, the resource type order is kinda special anyway.

Do you think it has a significant performance impact to wire it against the interfaces? I still feel it would be more consistent.

[Akii]

Another "weird" thing is that there is no builder for ReturnItem. I'm unsure why.

[jenschude]

I think the main reasoning was to avoid an additional layer of indirection.

That sounds a lot like premature optimization. There is a lot of indirection in the library already imo, and this would be "more consistent".

Totally. Just said what may have been the initial reasoning not that I share it. Especially when I look at the v1 SDK which adds the deserialization annotation at the subtype interfaces.

The specific reason is that 2 mixins are required if you'd want to replace the implementation of either of the two subclasses.

That's true.

To be fair, I don't know if there is an actual use-case since I'm not familiar with the underlaying business cases. I'm just looking at the technicals here, and the general look-and-feel of the SDK.

Then again, the resource type order is kinda special anyway.

I don't understand what you are meaning here

Do you think it has a significant performance impact to wire it against the interfaces? I still feel it would be more consistent.

Jackson is already one of the slower JSON serializer/deserializers. Which is not surprising giving it's rich feature set. And I don't know it's internals that good how it optimizes subtyping and deserialization.

Another "weird" thing is that there is no builder for ReturnItem. I'm unsure why.

Cause it makes no sense to build the instance of an interface which is polymorphic. And you are always working with the instance of an subtype. With the latest version there is a ReturnItemBuilder which instantiates the subtype builders.

[Akii]

I agree with all above points, but I have a question about this:

Another "weird" thing is that there is no builder for ReturnItem. I'm unsure why.

Cause it makes no sense to build the instance of an interface which is polymorphic. And you are always working with the instance of an subtype. With the latest version there is a ReturnItemBuilder which instantiates the subtype builders.

If I understand you correctly, you only ever work with the subtypes of ReturnItem. However, there is a default implementation for ReturnItem. Is ReturnItemImpl just unused?

[jenschude]

This is an artifact of the generated SDK. The polymorphic interfaces were initially abstract classes. But there are some edge cases were they make sense also for forward compatibility.

[Akii]

I'm in favour of closing the issue. It's not a big deal and can be worked around with the mixin just fine.
Because of that I'm closing the issue now but feel free to reopen it if you think it's worth fixing now.

Thank you for your swift response, I really appreciate it!