commercetools / commercetools-sdk-java-v2

The e-commerce SDK from commercetools for Java.
https://commercetools.github.io/commercetools-sdk-java-v2/javadoc/index.html
Apache License 2.0
34 stars 15 forks source link

Request for feedback: commercetools Java SDK custom types #264

Closed Akii closed 2 years ago

Akii commented 2 years ago

Hi,

I've written a library that extends the commercetools Java SDK by custom types defined in commercetools projects.

https://github.com/Akii/commercetools-sdk-java-v2-custom-types

The overall goal of the library is to provide as much type-safety as possible. To accomplish this, it introspects the types defined in commercetools projects and generates Kotlin data classes. It also provides a Jackson ApiModule for deserialization.

The generated code confirms to the SDK interfaces and by doing so does not introduce breaking changes I know of.

Thanks to how you structured the API it was possible to build this in a rather elegant manner.

I appreciate your feedback on this approach greatly.

jenschude commented 2 years ago

I really like it.

Especially the use of a gradle plugin. What I'm curios about is are the downloaded product types json cached, so that it will not be updated on every build? How would you deal with more than let's say 500 product types. Will the plugin support paging?

Small remark to the readme. The Attribute accessor can be also used in a more approachable way with the latest version (8.0.0-Beta.1/2):

variant.withProductVariant(AttributesAccessor::of).asString("text")

Btw it could be sufficient to create a map of field name to their field type as the type has to be the same for all attributes/custom fields across all product/custom types for a given name. But this could end up in a class with thousands of fields when dealing with hundreds of types.

And I just had another idea could it be possible to generate a type safe accessor like:

variant.withProductVariant(TestProductTypeAccessor::aBoolean) instanceof Boolean
variant.withProductVariant(TestProductTypeAccessor::anEnum) instanceof AttributePlainEnumValue
...

or

variant.withProductVariant(TestProductTypeAccessor.of()).aBoolean() instanceof Boolean
variant.withProductVariant(TestProductTypeAccessor.of()).anEnum() instanceof AttributePlainEnumValue
jenschude commented 2 years ago

And another thing what could be interesting. How is the interoperability of kotlin data classes with Java. Would it be possible to use them also in java easily. I had recently some fun dealing with them while trying to deserialize some XML to a data class using jackson and in the end I used POJOs

Akii commented 2 years ago

First of all thank you for your kind feedback!

What I'm curios about is are the downloaded product types json cached, so that it will not be updated on every build?

That one surprised me but Gradle actually handles that internally. Unless calling ./gradle clean, it will assume the downloaded file is up-to-date.

Will the plugin support paging?

I think this should already paginate, at least the function signature of the ct SDK suggests it: https://github.com/Akii/commercetools-sdk-java-v2-custom-types/blob/v0.0.2/commercetools-sdk-java-api-customtypes-gradle-plugin/src/main/kotlin/de/akii/commercetools/api/customtypes/plugin/gradle/actions/FetchProductTypesAction.kt#L27

That however raises another interesting question: Do you really still do case analysis if you need to differentiate between 500 types? Probably not. The library shines when you want to know which attributes are available, and what their types are. If you use a lot of nested types, it can be really, really benefitial.

Small remark to the readme. The Attribute accessor can be also used in a more approachable way with the latest version (8.0.0-Beta.1/2)

Good point! I will update the readme to reflect that. I don't think that v8 changes anything with regard to refactorability and type-safety of attributes (because attribute names are still just string references) but it ceratainly improves accessing values.

How is the interoperability of kotlin data classes with Java.

I actually think the interoperability is great! However you need to be aware of how Kotlin compiles to Java. For instance, with many XML libraries I find they require an empty default constructor. Normally Kotlin does not create a default constructor, unless every constructor argument has a default value.

The generated product attributes here are basically POJOs with a default constructor because commercetools cannot guarantee that required attributes are actually there (because otherwise you would not be able to add a new attribute).

As for using this library, you're good in a Java project as long as you include the kotlin std lib. I will certainly need to add documentation for that and do some testing.

As for your idea with the accessor: I need to think this one through some more. Looks interesting. But I wonder how that works with nested types and reference expansion.

The goal is really to create a complete structure that connects all ressources. Big maybe: Maybe we can even have something like lazy loading in the future. Just a thought (I usually hate that kind of magic).

jenschude commented 2 years ago

First of all thank you for your kind feedback!

What I'm curios about is are the downloaded product types json cached, so that it will not be updated on every build?

That one surprised me but Gradle actually handles that internally. Unless calling ./gradle clean, it will assume the downloaded file is up-to-date.

It could be good to define an output folder for the type definitions as well for the generated classes. So it would be possible to commit them. This could help reviewing changes if necessary.

Will the plugin support paging?

I think this should already paginate, at least the function signature of the ct SDK suggests it: https://github.com/Akii/commercetools-sdk-java-v2-custom-types/blob/v0.0.2/commercetools-sdk-java-api-customtypes-gradle-plugin/src/main/kotlin/de/akii/commercetools/api/customtypes/plugin/gradle/actions/FetchProductTypesAction.kt#L27

That's good that you used the QueryUtils.queryAll as it automatically pages through all resources.

That however raises another interesting question: Do you really still do case analysis if you need to differentiate between 500 types? Probably not. The library shines when you want to know which attributes are available, and what their types are. If you use a lot of nested types, it can be really, really benefitial.

I already saw projects with more than 1000 product types e.g. they were generated based on a multi dimensional matrix of attribute combinations.

Small remark to the readme. The Attribute accessor can be also used in a more approachable way with the latest version (8.0.0-Beta.1/2)

Good point! I will update the readme to reflect that. I don't think that v8 changes anything with regard to refactorability and type-safety of attributes (because attribute names are still just string references) but it ceratainly improves accessing values.

True that. It was more that the accessors are helpers to have type safe inline access.

As for your idea with the accessor: I need to think this one through some more. Looks interesting. But I wonder how that works with nested types and reference expansion.

It would be possible to do this already by manually writing such an accessor for your project, but it may be tedious to keep it in sync with the project configuration.

The goal is really to create a complete structure that connects all ressources. Big maybe: Maybe we can even have something like lazy loading in the future. Just a thought (I usually hate that kind of magic).

I don't see the need atm for lazy loading. The v1 SDK btw deserialized attributes, custom fields as JsonNode and they had to be mapped with the accessors. The v2 already deserializes the attributes according to there type

Akii commented 2 years ago

It could be good to define an output folder for the type definitions as well for the generated classes. So it would be possible to commit them. This could help reviewing changes if necessary.

You're right. I will add that and also make it configurable how class/property names are computed.

Thank you again for all your feedback! I will be closing the issue now because, well, it's not an issue of the SDK :D