jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
191 stars 55 forks source link

incorrect generic typing in operations of EntityGraph #424

Closed gavinking closed 12 months ago

gavinking commented 1 year ago

There are several problems in the EntityGraph interface:

  1. Calling addAttributeNodes(Attribute<T, ?>... attribute) produces the infamous "Unchecked generics array creation for varargs parameter" compiler warning.
  2. addAttributeNodes(Attribute<T, ?>... attribute) should be addAttributeNodes(Attribute<? super T, ?>... attribute)
  3. <X> Subgraph<X> addSubgraph(Attribute<T, X> attribute) should be<X> Subgraph<X> addSubgraph(Attribute<? super T, X> attribute)
  4. I'm not 100% sure, but I believe that the signature of <X> Subgraph<X> addKeySubgraph(Attribute<T, X> attribute) is simply wrong. I think it should be <X> Subgraph<X> addKeySubgraph(MapAttribute<T, X, ?> attribute).

We need to put some thought into these.

gavinking commented 1 year ago

Additionally, I'm very confused by this method:

    /**
     * Add a node to the graph that corresponds to a managed
     * type with inheritance.  This allows for multiple subclass
     * subgraphs to be defined for this node of the entity
     * graph. Subclass subgraphs will automatically include the
     * specified attributes of superclass subgraphs. 
     *
     * @param attribute  attribute
     * @param type  entity subclass
     * @return subgraph for the attribute
     * @throws IllegalArgumentException if the attribute's target 
     *         type is not a managed type
     * @throws IllegalStateException if the EntityGraph has been 
     *         statically defined
     */
    public <X> Subgraph<? extends X> addSubgraph(Attribute<T, X> attribute, Class<? extends X> type);

The Javadoc is extremely cryptic, and the signature is weird. Is it supposed to be doing the equivalent of a treat()? If so, shouldn't the signature be:

public <X, Y extends X> Subgraph<Y> addSubgraph(Attribute<T, X> attribute, Class<?Y> type);

because with the present signature it doesn't seem like you could do anything useful with it, assuming my interpretation is correct.

gavinking commented 1 year ago

I'm also confused by this one:

public <X> Subgraph<? extends X> addSubclassSubgraph(Class<? extends X> type);

which actually makes no sense even without knowing the semantics: Java has wildcard capture, so you don't need the wildcards there.

Perhaps it should be:

public <X extends T> Subgraph<X> addSubclassSubgraph(Class<X> type);
gavinking commented 1 year ago

Please see https://github.com/jakartaee/persistence/pull/425 for one set of ideas for how to "fix" all this.

lukasj commented 1 year ago

this supersedes #411