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

Use LinkedHashMap to preserve order of schema elements #395

Open
JoshMcCullough opened this issue Jan 7, 2021 · 5 comments · May be fixed by #467
Open

Use LinkedHashMap to preserve order of schema elements #395

JoshMcCullough opened this issue Jan 7, 2021 · 5 comments · May be fixed by #467

Comments

@JoshMcCullough
Copy link

JoshMcCullough commented Jan 7, 2021

Currently a HashMap is used, so the order of e.g. properties is not preserved. A simple change to use LinkedHashMap instead would preserve the schema designer's insertion order.

Example:

ObjectSchema.builder()
    .addProperty("one", ...)
    .addProperty("two", ...)
    .addProperty("three", ...);

The result in something like ...

{
  "properties": [
        "two": ...,
        "three": ...,
        "one": ...,
  ]
}

... where the properties are in an indeterminate order (based on the hash of the key). Preferably, the order would be preserved:

{
  properties: [
        one: ...,
        two: ...,
        three: ...,
  ]
}

Some would argue "no way man, the JSON spec specifically says that property order doesn't matter" (yeah, I lost that fight and here I am again for round 2), but I'd argue that the order being effectively random is far worse than the order being preserved.

@erosb
Copy link
Contributor

erosb commented Jan 8, 2021

Hello Josh,

I'm not strongly against the ordering, but I find using LinkedHashMap risky (there are some users who use extremely large generated schemas, and using LinkedHashMap could cause a significant performance loss for them). So if your usecase is only for "properties", then I suggest adding an ObjectSchemaBuilder#preserveOrder() method that would change the internal representation from HashMap to LinkedHashMap. What do you think about that? Would it be sufficient for your usecase? (By the way I can't promise I will implement it in the near future).

@JoshMcCullough
Copy link
Author

LinkedHashMap generally performs better than HashMap, the only downside is the small amount of additional memory it would use to store the "links". I'd suggest it isn't necessary to "choose one or the other", but a straightforward performance test would answer that question.

@hiloboy0119
Copy link

Hey Josh, not sure if this will help you but this is how I solved this problem:

import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.Map;
import org.everit.json.schema.ObjectSchema;
import org.everit.json.schema.Schema;

public class OrderedObjectSchema extends ObjectSchema {

  public static class Builder extends ObjectSchema.Builder {
    private final Map<String, Schema> orderedPropertySchemas = new LinkedHashMap<>();

    public Builder addOrderedPropertySchema(String propName, Schema schema) {
      super.addPropertySchema(propName, schema);
      orderedPropertySchemas.put(propName, schema);
      return this;
    }

    @Override
    public OrderedObjectSchema build() {
      return new OrderedObjectSchema(this);
    }
  }

  private final Map<String, Schema> orderedPropertySchemas;

  /**
   * Constructor.
   *
   * @param builder the builder object containing validation criteria
   */
  public OrderedObjectSchema(Builder builder) {
    super(builder);
    orderedPropertySchemas = Collections.unmodifiableMap(builder.orderedPropertySchemas);
  }

  @Override
  public Map<String, Schema> getPropertySchemas() {
    return orderedPropertySchemas;
  }
}

@JoshMcCullough
Copy link
Author

@hiloboy0119 Thanks -- looks about right to me at first glance. I'll give it a shot when I get a chance.

@JoshMcCullough
Copy link
Author

@hiloboy0119 I think that would be okay as a workaround but I still say it should be updated in the library itself as preserving order is better than random order. I'll make a PR at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants