GetEntityGraphAttributePaths() Method on DynamicEntityGraph

[austinarbor]

Not so much a bug, but in the legacy EntityGraph and DynamicEntityGraph classes there was a method getEntityGraphAttributePaths() to return back the attribute paths. I have a lot of code dependent on this method for unit tests and for merging multiple graphs together. Can you add this method back in for the new DynamicEntityGraph model, or alternatively make the field protected instead of private so I can extend the class and access the field without reflection?

Thanks!

[reda-alaoui]

alternatively make the field protected instead of private so I can extend the class and access the field without reflection

You can already do that by wrapping DynamicEntityGraph into your custom class like this:

class MyDynamicEntityGraph implements EntityGraph {
  private final DynamicEntityGraph delegate;
  private final List<String> attributePaths;

  public MyDynamicEntityGraph(EntityGraphType type, List<String> attributePaths) {
    this.delegate = new DynamicEntityGraph(type, attributePaths);
    this.attributePaths =
        Optional.ofNullable(attributePaths)
            .map(ArrayList::new)
            .map(Collections::unmodifiableList)
            .orElse(Collections.emptyList());
  }

  @Override
  public Optional<EntityGraphQueryHint> buildQueryHint(
      EntityManager entityManager, Class<?> entityType) {
    return delegate.buildQueryHint(entityManager, entityType);
  }

  public List<String> getEntityGraphAttributePaths() {
    return attributePaths;
  }

}

Isn't that enough? I don't want to break encapsulation only to allow unit tests.

for merging multiple graphs together

We could add this feature directly to the 'new' DynamicEntityGraph provided by this project without breaking encapsulation. Could you create a pull request for this? It would be used like this:

DynamicEntityGraph merged = dynamicEntityGraph1.merge(dynamicEntityGraph2)

and/or

DynamicEntityGraph merged = DynamicEntityGraph.merge(dynamicEntityGraph1, dynamicEntityGraph2, dynamicEntityGraph3);

[austinarbor]

You can already do that by wrapping DynamicEntityGraph into your custom class like this

Sure but then I am basically maintaining duplicated logic as the library and also have to store a duplicate copy of the paths...obviously it's not a lot of work but just seems unnecessary when a getter on the paths would be enough.

I'm assuming by breaking encapsulation you mean you don't want someone to mutate the list after calling the getter? Could you return a copy of the list, or use an immutable list to store it to prevent this?

[reda-alaoui]

I'm assuming by breaking encapsulation you mean you don't want someone to mutate the list after calling the getter? Could you return a copy of the list, or use an immutable list to store it to prevent this?

DynamicEntityGraph is already immutable with an unmodifiable list. Encapsulation also includes hiding internal structure of the object. If the list is exposed publicly, it will be hard to use a different internal storage. Exposing internal structures only to allow unit test is not a good policy. So I won’t do it, sorry.

Therefore, for the unit test problematic, you can either:

• extend DynamicEntityGraph
• wrap DynamicEntityGraph as I shown
• turn this to integration test
• mock EntityManager to verify the received calls
• remove the unit test if it only tests the merge feature of your graphs and transfer this feature to this project

For the merge problematic, we can add this feature here without exposing internal structure.