commercetools / commercetools-jvm-sdk

The e-commerce SDK from commercetools running on the Java virtual machine.
https://commercetools.github.io/commercetools-jvm-sdk/apidocs/index.html
Other
62 stars 40 forks source link

Custom Fields of Type Set ordered but only available as Java Set. #1319

Open nkuehn opened 7 years ago

nkuehn commented 7 years ago

The custom Field type "Set" is defined as

The set field type defines a set (array without duplicates) with values of the given elementType. ( http://dev.commercetools.com/http-api-projects-types.html#settype )

I.e. going along JSON semantics it contains and keeps ordering information (an "Array" is generally considered ordered). But the JVM SDK only provides helpers that expose it as a Set, e.g. a Set of Strings, losing the ordering information (probably accessible be digging into the Json Tree).

I propose to add an accessor List etc. that keeps the ordering information (and check back with the platform team - the topic is a constant source of confusion).

katmatt commented 7 years ago

We use the java.util.LinkedHashSet as set implementation and this implementation maintains the order of the entries. I also added an integration test for it (https://github.com/commercetools/commercetools-jvm-sdk/blob/custom-field-set-type-is-ordered/commercetools-models/src/test/java/io/sphere/sdk/categories/CategoriesCustomFieldsIntegrationTest.java#L51). But I agree with you that we should make it more explicit that the set keeps the ordering.

nkuehn commented 7 years ago

But why is the external interface a and not a then? you don't lose anything.

katmatt commented 7 years ago

In java a java.util.Set can keep the order of its elements and as a developer you have to decide if you want to keep the order. But you don't have to declare the concrete java.util.Setimplementation that you are using (in our casejava.util.LinkedHashSet`). This is considered a good practice because you don't expose any implementation details . And the JDK unfortunately has no special interface for sets that keep the ordering of their elements. So I would suggest to add javadoc that explains this further.

selo-ecube commented 7 years ago

While it is technically possible that a Set retains a given order, and LinkedHashSet does so, the JavaDoc of java.util.Set states that the Set "interface models the mathematical set abstraction." - which is not generally expected to cover any order.

Given that the abscence of order in Set instances is a nobrainer in Java, I doubt that users of the JVM SDK would to try to open JavaDoc for this aspect. (I wouldn't, at least.)

I agree that it is by convention not desireable to return a LinkedHashSet<String> instead of a Set<String>. What about providing a hint in the names of the methods that return the set (maybe in combination with JavaDoc that explains the situation): e.g. Attribute.getValueAsOrderedStringSet():Set<String> instead of the then-deprecated Attribute.getValueAsStringSet():Set<String>, TypeReference.orderedStringSetTypeReference():TypeReference<Set<String>> for TypeReference.stringSetTypeReference():TypeReference<Set<String>> etc. for all the other kinds of sets?

lauraluiz commented 7 years ago

The problem is that a couple of releases ago the order was lost because the JSON mapper used the HashSet implementation. That was definitely wrong. But now it's not losing ordering anymore, and although Set might not be explicit about the order keeping, just avoiding duplicates, it's not going against it either as it all depends on the implementation you use.

As a user, I would assume that if the HTTP API documentation states that a it's an "ordered set" (i.e. array without duplicates), the only equivalent implementation in the standard Java library is a LinkedHashSet, so a Set in the end, and I would assume everything is correct (until proved otherwise like it happened 2 releases ago).

So in short, from my point of view it is correct as it is now and it doesn't need further explanation, but if it's a general concern then I would consider the renaming of the methods, which is the best approach. Only issue is that in general the method and class naming from the JVM SDK corresponds to the HTTP API documentation, so in that sense "Set" is correct too.

nkuehn commented 7 years ago

@lauraluiz ~~sorry, no. Java has a well defined interface for what we call an "ordered set" (in "Math speak"). In "java speak" that's a SortedSet
https://docs.oracle.com/javase/7/docs/api/java/util/SortedSet.html~~

I am not deep into the java implementation details you're all talking about above but on the logical level there is absolutely no ambiguity. A Set interface is not sorted and should not be treated under the assumption to be sorted by application code.

lauraluiz commented 7 years ago

@nkuehn sorted does not mean the same as ordered. Sorted means that it has an algorithm behind that determines where to place a new instance when you add it to the set. Our use case has no algorithm behind, it's just arbitrarily ordered after the merchant's wishes. Moreover, you'll notice that LinkedHashSet (which is the implementation that reflects our use case perfectly) does not implement SortedSet.

So the Java API doesn't provide any interface for an ordered set, which we can discuss whether it's correct or not, but Set is the only thing we can return here.

Then the question is whether we should just simply document our API the same way it was done in the platform's API (they are returning Set, but documenting it's actually an ordered set) or we should go one step further and change our method names to be more expressive regarding this topic.

nkuehn commented 7 years ago

argh, you're right.

Sorry for the confusion, I'm jumping between languages and environments too much at the moment (trying to learn scala etc).

I know, it's not good style to define interfaces with collection implementations, but in this case LinkedHashSet seems to be the only implementation that has the desired properties, so why not expose it directly (at least as an overloaded alternative to get the data)? That would lead to everybody seeing it in the IDE directly without having to check the Javadoc.

Generally, if the documentation is part of the tooltips and / or autocompletes in IntelliJ we've already gone a long way.

PS/ offtopic: Do you know why the language design doesn't have a default Interface for this case in set theory? It's a very common case in the real world domain.

katmatt commented 7 years ago

Then I would suggest that I add some javadoc to the corresponding methods. And Intellej/Eclipse are able to show this documentation.

Offtopic: One possible reason that the JDK doesn't provide an OrderedSet interface is that such an interface wouldn't have any additional method. And as our discussion shows the difference between ordered and sorted isn't very clear - especially if you work with a lot of different programming languages. And the JGL java library, which existed before the java.collections API, calls a sorted set OrderedSet web.stanford.edu/group/coursework/docsTech/jgl/api/com.objectspace.jgl.OrderedSet.html 👎