Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clarification] should PropertyOrderStrategy also get applied to Map and Set fields? #78

Open
jsonbrobot opened this issue Apr 16, 2018 · 4 comments
Labels
documentation Involves the spec or javadocs enhancement New feature or request

Comments

@jsonbrobot
Copy link

We've got a user question in Johnzon whether setting PropertyOrderStrategy should end up sorting Map and Set fields as well.

My personal interpretation from reading the spec is that it only applies to class fields (JSON-B properties).

public class Bla {
  private String b = "bla;
  private Map<String, Integer> a = new HashMap<>();
  ...
  a.put("x", 4);
  a.put("y", 5;
  ...
}

Might end up as

{"a":{"y":5,"x":4},"b":"bla"}

Note that while the field members are sorted (a,b) the Map values are not.

Something to keep in mind that Map, Set, etc most times already have a very distinct ordering which is depending on the concrete implementation. E.g. if we have a HashMap then it is always unsorted. And even if we would call the add() operations in the 'correct' order then it will still end up random in the HashMap. Same applies to TreeMap which is always sorted after the key.

So it might only have an impact on the writer side anyway, isn't?
And there it might cause a useless performance impact in case we don't need the sorting.
Because the default ordering in JSON-B is Lexigraphical, isn't?
So that would mean that we would not be able to just write out the Map but first would need to sort the keys and then write out the values in that order. Sounds a bit too much for me in most cases.

We could probably introduce another config flag to indicate default sorting of collections?

What about order applied as annotation to a Map or Set directly?

@jsonbrobot
Copy link
Author

@bravehorsie Commented
For class properties sorting can be done during initialization / class scanning. Sorting collections in runtime will impact performance significantly if done by default.

Collections has their sorting contract well defined outside of JSONB, it does not sound reasonable to me to override it.

For now we are ordering just the keys in json document which relates to Map implementations, but if we declare that we override collection sorting what will we do about non-keyed ones - comparing toString result?

@jsonbrobot
Copy link
Author

@rmannibucau Commented
I think the idea is more the following:

  1. if the map is sorted -> respect the map ordering
  2. if the map is not sorted -> use the global PropertyOrderStrategy

I agree it can require a flag to activate 2 which has performance impacts on big maps.

@jsonbrobot
Copy link
Author

@aguibert
Copy link
Contributor

I think this is a valid expectation. In Yasson we use a HashMap(unsorted) by default, but if the sorted strategy is specified we can use a sorted collection like TreeMap instead.

We should clarify this in the spec and add a TCK test for it. @bravehorsie where do we keep the JSON-B TCK tests?

@aguibert aguibert added question Further information is requested enhancement New feature or request documentation Involves the spec or javadocs and removed question Further information is requested labels Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Involves the spec or javadocs enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants