Handling of Typed Collections

This is a trade-off in where we expect our interoptibility to occur:

Child interoptibility:

• by providing getFoos(): Collection<Foo> GeoAPI origionally intented to replace, addFoo( Foo ), getFoo( i ), addFoo( int , Foo ) etc ...
• the Collection<Foo> implementations used were expected to handle any implementation of Foo provided by client code
• if a Collection implementation (for an OR mapper) needed needed a particular instance they can copy the child (as a data object)

The down side is that our interfaces are not defined as data objects (we would need to define the hashcode and instanceof in javadocs)

AbstractFactory interoptibility:

• as outlined in the change request interoptibility is captured at the level of an an AbstractFactory
• the instances used are compatible with the host application (indeed provided by the host application via injection)

From email Gabriel:
I guess the balance will be in using the form <? extends Interface> only for
those classes defined as abstract in the specification.
See for example the DQ_Element hierarchy, where DQ_Element is abstract, so
DataQuality.getScope() should return <? extends Element> and not just
Element.

This is a concern we did not got yet as geotools is 1.4, but definately needs
to be addressed.

By the way, while updating the geotools implementation I found that all the
classes declared abstract in the spec were concrete in the implementation. I
guess they should be abstract too. Like I found a couple places in the
referencing module where they were being directly instantiated which I guess
is wrong.

From email Martin:

The later is also a cause of problems. The way GeoAPI metadata
interfaces are currently defined, there is no

Citation.addPresentationForm(...);

method. Users are encouraged to write:

Citation.getPresentationForms().add(...);

This avoid the need to pollute the API with add, get, contains, indexOf,
etc. methods that duplicates (in a less powerfull way) all the methods
already provided in Java collections.

However, Citations.getPresentationForms.add(...) doesn't work with <?
extends Foo> generic type, because the compiler can't ensure that the
operation is safe. In summary:

<? extends Foo> allows read operations, no write operations.
<? super Foo> allows write operations, no read operations.
<Foo> allows both.

I'm not completly sure that I'm not wrong, since I do not play with
generic types as much as I would like (because Geotools is still
restricted to Java 1.4. Sight...). If I'm not wrong, I think that we
should really stick to <Foo> generic type declaration for non-abstract
interfaces, as suggested by Gabriel.

I attached to this issue a ZIP file with three packages:

• org.opengis.spike.plain
• org.opengis.spike.covariant
• org.opengis.spike.contravariant

Each package contain the following classes / interfaces:

• interface ResponsibleParty
• class ResponsiblePartyImpl
• interface Citation
• class CitationImpl
• main class Test

Those classes are identical in each package, except that Citation declares the getCitedResponsibleParties() method as bellow:

• In org.opengis.spike.plain:

  Collection<ResponsibleParty> getCitedResponsibleParties();

• In org.opengis.spike.covariant:

  Collection<? extends ResponsibleParty> getCitedResponsibleParties();

• In org.opengis.spike.contravariant:

  Collection<? super ResponsibleParty> getCitedResponsibleParties();

Look at the Test class in each of those packages. In org.opengis.spike.plain,
we can read and write in the collection. In org.opengis.spike.covariant, we
can only read. In org.opengis.spike.contravariant, we can only write. The
opposite operation is a compiler error.

Thanks for the example martin ... I am just going to take some notes here on how we can keep our API and solve the problem.

The origional request basically amounts to "make it so I don't have to cast".
And our answer seems to be "Cast? Are you crazy..."
Resulting in a stalemate "Huh? But I need to map these to the database"

Here is the example:

class CitationImpl implements Citation {
    @OneToMany(mappedBy = "citation", cascade = CascadeType.ALL)
    @OnDelete(action = OnDeleteAction.CASCADE)
    @Relationship
    private Set<CitationDateImpl> dates = new HashSet<CitationDateImpl>();
...
    public Collection<CitationDateImpl> getDates() {
        return dates;
    }
...
}

And here is what we need to do to fix it:

class CitationImpl implements Citation {
    @OneToMany(mappedBy = "citation", cascade = CascadeType.ALL)
    @OnDelete(action = OnDeleteAction.CASCADE)
    @Relationship
    private Set<CitationDateImpl> dates = new HashSet<CitationDateImpl>();
    private SafeSet<CitationDate> safeDates = new SafeSet<CitationDate,CitationDateImpl>( dates );
...
    public Collection<CitationDate> getDates() {
        return safeDates;
    }

Where SafeSet" has an implementation of add that:

• treats CitationDate as a data object (strick equals and hashcode)
• will add any direct implementation of CitationDateImpl to the wrapped set
• will copy out the values from a CitationDate into a CitationDateImpl and then add

This approach only works for pure data objects (no subclasses of CitationDate need apply).

Yes, its look like that such SafeSet could work.

Unfortunately such a SafeSet will not work since Hibernate uses reflection to set the 'dates' field directly, so the SafeSet will contain stale data. A more complex approach that actually reads the field using an anonymous inner class would work.

But I have some other remarks about the generic collections:

For our implementation, where we want to map classes to a relational database using EJB3 annotations, the fixed generic collections (like Collection<CitationDate>) is actually the most troublesome approach because it prevents us from overriding the return value. Using a non-generic collection or something like Collection<? extends CitationDate> works better because then we can override the method return value in the implementing class to return something like Collection<CitationDateImpl>. The advantages of this approach are:

• No need for a complicated conversion.
• We can have matching getters and setters; the ISO interfaces don't have setters in most cases.

This does not change the issues mentioned above for someone who is working directly on the interfaces, but at least a less restrictive return value lets us overcome some of the limitations of Java generics in the concrete implementation.

Thanks for the correction Felix - indeed we will need the "SafeSet" to hold on to the CitationImpl so it can always delegate to the dates field - this can be quickly done with an anonymous inner class. Such limitations/opportunities are specific to the persistence technology used for implementation - from the standpoint of GeoAPI we need to focus on making sure the client code is free from any awareness or consequence of the persistence technology.

From an architecture standpoint Hibernate has automated most of the DTO tasks that traditionally are done as part of using an ER mapper. If a CitationDateImpl was allowed to "leak out" of the persistence layer into client code it would represent a break with the layered architecture promoted in a J2EE environment.

Which is not to say it does not happen - Spring Remoting introduces a couple new runtime exceptions and restrictions on the equals method beyond pure layer boundaries. I just expect better from Hibernate; and I suppose up until Java generics they were comfortable hiding behind Collection and List.

We can prep up the anonymous inner class example of SafeSet as a next step I guess.

You are clear about why this issue is moving towards WONT FIX?

• implementations are few in number, is the 10% use case, and if they are using an ER mapper I expect the developers to be on the ball and careful
• client code is large in number, is the 90% use case, and I expect it is done against a deadline

This is an odd case and I am hard pressed to think that Hibernate put forth this use of Generics as one of its best practises.

Actually I think the real issue is the (unnecessary) restrictions in Java generics, i.e. that you can't add to a Collection<? extends X>. Hibernate is doing nothing wrong, they just implement the EJB3 standard.

But there is one more serious problem for our implementation of the interfaces: If the method return types are changed to generic collections, the interfaces are not backwards compatible! This is so because we currently override the non-generic return types to become generic collections of our implementation objects. So now we have to rewrite our hundred existing implementation classes and the code that uses them?

I have been fussing over this issue for a couple of days; and I have had an insight.

The whole thing we do with GeoAPI is create interfaces for communication. Mutability tends to stand in the way of that (usually it is a lot more indirect then what we have here with generics and collections).

Why GeoAPI should be "readonly":

• synchronization is never a good time and cannot be done at an interface level
• adding events is a bad idea, see what happened to GeoTools style interfaces when events need to cascade (the result will never work for filter - it is a bad idea)
• and mutability may enforce constraints on implementations that are unsuitable (such as with our current issue)

So with this in mind I would like to see a bit of a formula written up on how to turn a specification into interfaces based on what we have figured out during GEO-93.

We should change the interfaces to Collection<? extends X.> as this does keep client code from changing the collection. This is a good thing and in accordance with readonly interfaces. Further more it does offer implementations the power to be mutable if they so wish - for GeoTools the Collection<XXXImpl> is a good thing and would let those implementations show how exactly they can be edited (so the freeze() can cascade and freeze the entire tree).

For those playing games with an OR mapper they also can control the implementation used.

In both cases GeoAPI does not say how the values may be modified (either set methods or via database) - it is not our game.

Rant over

A fair question from Bryce (via email):
How does a FeatureReader/CoverageReader plugin construct metadata in an
implementation independent fashion with these interfaces? If it's covered
via factories, OK. If not, boo hiss.

This bug report is not about factories, currently nobody has defined factories for metadata infterfaces - this can be done if there is interest. Right now metadata providers are responsible for providing their own implementation (however they see fit: both OR mappers and mutable implementations are available in the wild)

In answer to a previous comment, the fact that you can't add to a Collection<? extends X> is not a unnecessary restriction in Java generics. It is an intented design based on pure logic. Such operation would be unsafe and break the whole purpose of Java generics. Example:

class Animal { ... }
class Cat extends Animal { ... }
class Dog extends Animal { ... }

Set<? extends Animal> animals;
animals = new HashSet<Cat>();
animals.add(dog);

If add was allowed in Collection<? extends Animal>, then the add in the code above would be wrongly accepted by the compiler because Dog is an animal. The compiler don't know that the animals set is restricted to Cat since animals is declared as Set<? extends Animal>.

I don't know Hibernate well enough, so I can't claim that it is doing something wrong. But the question seems open to me. Why hibernate mandates a particular implementation? Taking the example code provided above in this thread, why the following can't work with hibernate?

class CitationImpl implements Citation {
    @OneToMany(mappedBy = "citation", cascade = CascadeType.ALL)
    @OnDelete(action = OnDeleteAction.CASCADE)
    @Relationship
    private Set<CitationDate> dates = new HashSet<CitationDate>();
...
    public Collection<CitationDate> getDates() {
        return dates;
    }
...
}

committed r989

Just to be clear: replacing Collection<X> by Collection<? extends X> do NOT gives more freedom. It replaces a constraint by an other constraint. And Java generic types is not unnecessary restrictive in this regard.

I'm concerned because we are abandonning a model flexibility (allow write operations if implementor wish) in favor of an implementation-specific flexibility (allow exposition of implementation-specific classes). This change seems against the interoperability spirit of an interface-only project to me. Especially if this change was done in order to please a particular implementation which has a limitation that I don't understand at this time (why Hibernate can't work with Set<CitationDate> instead of Set<CitationDateImpl>?).

If Hibernate really wants Set<CitationDateImpl>, I believe that a workaround exists. Please consider the following, which is type-safe (note that Collections.unmodifiableSet(...) expects a Set<? extends T> argument and returns a Set<T> object, which is exactly what we need and is legal because it returns a read-only view of the Set):

class CitationImpl implements Citation {
    @OneToMany(mappedBy = "citation", cascade = CascadeType.ALL)
    @OnDelete(action = OnDeleteAction.CASCADE)
    @Relationship
    private final Set<CitationDateImpl> dates = new HashSet<CitationDateImpl>();

    private final Set<CitationDate> datesAPI =
        Collections.unmodifiableSet((Set<? extends CitationDate>) dates);
...
    public Collection<CitationDate> getDates() {
        return datesAPI;
    }
...
}

If we want to returns a writeable Set instead of a read-only one in some future version, we can handle that with our own subclass of AbstractSet. We need only one such adapter class for the whole project (not one for each return type).

having another look

But if you say that the collections should allow write operations, then why doesn't the interface let me write to any of the other properties? I think Jody is correct in that limiting write access is consistent with the read-only approach of the interfaces.

The reason for the EJB3 (an its Hibernate implementation) need of implementation classes is that they do persistence. This means that they create Java objects with data they read from a database, and for that they need a concrete implementation, not just an interface. I guess it would be possible to write an ORM that just uses interfaces and factories, but I have never seen one and that would be a lot more work with very little benefit.

Actually, if you take persistence into account, making the collection interfaces writable is not a good idea, because it would allow people to add objects into a collection that come from completely different implementations of the interfaces, which would surely break persistence. From that point of view, I would say we actually need to replace Collection<X> with Collection<? extends X> so that people have to use implementation objects to create the metadata, thereby ensuring the consistent use of a particular implementation.

Unfortunately I can see some issues in your workaround idea:

• The Set<CitationDate> needs to be dynamically constructed because Hibernate will set its own implementation class in order to handle lazy loading. This would be doable with an anonymous inner class, but a simple implementation would construct a new collection for every call to the getDates().
• It forces us to write a second getter if we want access to the collection of implementation objects, and what should we call that?
• Why would we return an unmodifiable set? I though you want to be able to write to the collection.

Here is a corrections to Martins code example:

class CitationImpl implements Citation {
    @OneToMany(mappedBy = "citation", cascade = CascadeType.ALL)
    @OnDelete(action = OnDeleteAction.CASCADE)
    @Relationship
    private final Set<CitationDateImpl> dates = new HashSet<CitationDateImpl>();

    public Collection<CitationDate> getDates() {
        return Collections.unmodifiableSet((Set<? extends CitationDate>) dates);
    }
...
}

Felix if this is a read-only interface (for publication of information into a possibly multi-threaded environment we do not want to see the contents of the set change).

I am trying to break down the different use stories here:

• http://docs.codehaus.org/display/GEO/Interfaces

The question moves on to how an application can make its own mutable implementation avaialble to GeoAPI (since GeoAPI does not allow editing this should be easy, anyone using the pure GeoAPI interfaces acts as a "reader", an application can take resonsibility for providing a specific implementation for writers (with synchornization, set methods, and so on ... without bothering anyone else).

class CitationImpl implements Citation {

    String isbn;

    @OneToMany(mappedBy = "citation", cascade = CascadeType.ALL)
    @OnDelete(action = OnDeleteAction.CASCADE)
    @Relationship
    private final Set<CitationDateImpl> dates = new HashSet<CitationDateImpl>();

    public synchronized String getISBN(){
          return isbn;
    }
    public synchronized String setISBN( String isbn){
          this.isbn = isbn;
    }
    public Collection<CitationDate> getDates() {
        return Collections.unmodifiableSet((Set<? extends CitationDate>) dates);
    }

   public synchronized Collection<CitationDate> getDates() {
        return Collections.unmodifiableSet((Set<? extends CitationDate>) getDateImpls);
    }
    public Collection<CitationDateImpl> getDateImpls() {
        return Collections.synchronizedSet(dates);
    }
   public synchronized void setDates( Collection<CitationDateImpl> dates ) {
        this.dates = dates;
    }
}

In answer to a previous comment ("if collections should allow write operations, then why doesn't the interface let me write to any of the other properties?"), it was a conservative approach, something that I do often: I don't put something in an interface when I'm unsure about it, on the basis that it is easier to fill a hole at some later stage than to fix something that was wrong in the first place. I'm in favor of read-only metadata at the GeoAPI interface level, but had no garantee that the community will not want to add "set" methods at some later stage. The room was left open, either for implementors or for some future GeoAPI version. But if we replace "Collection<X>" by "Collection<? extends X>", that room is lost. We are making one step toward commiting GeoAPI metadata interfaces to stay read-only in future releases.

It may or may not be a good thing; I'm not sure (how do we expect users to update their metadata? Do we take the decision that they will have to rely on specific implementations?). I guess it should be a community decision.

I don't have new arguments or workaround to suggest, so I guess that at this time I should let other users express their opinion, and let the decision to you since you probably did more experiment with metadata than me a this stage.

Note that the change discussed in this issue implies closing GEO-54 as "will not fix".