The code always knows

Posted by jive on February 20, 2007 at 11:45 PM EST

The code always knows Abstract: For the last couple of months the GeoTools community has been creeping around a problem ... one of quality. I am going to wade in and do something about it; a code reivew. The catch? It is probably going to be my own code.

The problem? We have some plugin implementations that do not follow a coding contact. Normally this is not such a big deal (you throw those plugins out of the build until they shape up). Problem is this time it is *every* plugin.

Unimplemented functionality - how did this Happen?

When this happens it usually means that we (the GeoTools community) have let a user request change our api (probably because some functionality is commonly requested), but we did not take the trouble to get buy in (and time) from the plugin implementors.

Without client code around at the same time as implementations are being built this kind of thing falls through the cracks (release early release often is the mantra, feedback early when developers care is the reality).

The DataStore API has suffered this same fate before; currently uDig only supports three DataStores. As a client application needs events to be fired when editing; but since uDig arrived on the scene a bit late later then the implementations it was not around to ensure this functionality worked as advertised.

Unimplemented functionlaity - why Fix it?

A really easy alternative is to just remove the functionality (it is not being implemented that should tell us something is in a WONT FIX state).

Here is why I am not going to do that ... hacking around this problem is producing the same boiler plate code in a number of spots:

In our Renderer .. I saw this back in November when we hooked up Expression (a query langague) so that POJOs (ie normal objects) could be drawn onto a map
In other applications .. GeoServer noticed this problem and talked about it in last weeks IRC meeting
In other applications .. uDig developers noticed and hacked around the problem so quickly they forgot to report it

It is good practice to only allow a hack three times; and it looks like we are at that number. Since we have three working hacks to draw on the fix should be easy, and removing the hacks and running the test cases should provide us confidence that we have done a good job..

Broken - using Query to Reproject

This is what a simple data access query looks like:

FeatureSource source = dataStore.getFeatureSource( "road" );
FeatureCollection features = source.getFeatures();

And here is one that uses a Query:

FeatureSource source = dataStore.getFeatureSource( "road" );
DefaultQuery query = new DefaultQuery( "road, Filter.INCLUDE );
FeatureCollection features = source.getFeatures();

And finally here is the problem - using Query to ask for something in another projection:

CoordinateReferenceSystem world = CRS.decode("EPSG:4326"); // world lon/lat
CoordinateReferenceSystem local = CRS.decode("EPSG:3005"); // british columbia
        

FeatureSource road = store.getFeatureSource( "road" );
DefaultQuery query = new DefaultQuery( "road", Filter.INCLUDE, new String[]{ "name" } );
        
query.setCoordinateSystem( local ); // FROM, optional, will ignore actual data and force CRS
query.setCoordinateSystemReproject( world ); // TO, optional, will reproject data into CRS
                
FeatureCollection features = road.getFeatures( query ); // will reproject the features

You can see why this is a popular request - it actuall does something a bit more then just data access.

setCoordinateSystem() - changes the "Metadata", the information will be returned as is (same values) but the meaning will be changed - the FeatureType will report back that the information is in the provided CRS
setCoordianteSystemReproject() - changes the actual "Data", in addition to changing the FeatureType the actual data values will be reprojected and the result returned

Solution? The code always knows...

The needed code is around, it has just not been hooked up behind that getFeatures( query ) method.
Here is the first HACK needed to set the FeatureType up correctly (from DefaultView.java)::

FeatureType origionalType = source.getSchema();

CoordinateReferenceSystem cs = null;
if (query.getCoordinateSystemReproject() != null) {
    cs = query.getCoordinateSystemReproject();
} else if (query.getCoordinateSystem() != null) {
    cs = query.getCoordinateSystem();
}
schema = DataUtilities.createSubType(origionalType, query.getPropertyNames(), cs, query.getTypeName(), null);

Here is the second HACK, the utility classes to do the hard work are right there, they just have not been connected.

if (forcedCS != null)
    results = new ForceCoordinateSystemFeatureResults(results, forcedCS);
if (reprojectCS != null)
    results = new ReprojectFeatureResults(results, reprojectCS);

So the solution is to "wrap" the origional feature collection in a helper class that does the work. You can see these same utility classes used in the Renderer, and in GeoTools and in ... your application. (that is the nature of a workaround).

First of All the Test Case

In order to debug a problem you need to be able to reproduce it. Here in java land that tends to mean a test case, for bonus points it means the failure will have a hard time coming back from the dead.

I am going to pick on a *really simple* data store that uses Java property files, simply because I am the implementor responsible for it and I feel guilty :-)

Here is the test case:

    public void testQueryReproject() throws Exception {
        CoordinateReferenceSystem world = CRS.decode("EPSG:4326"); // world lon/lat
        CoordinateReferenceSystem local = CRS.decode("EPSG:3005"); // british columbia

        FeatureSource road = store.getFeatureSource( "road" );
        FeatureType origionalType = road.getSchema();

        DefaultQuery query = new DefaultQuery( "road", Filter.INCLUDE,
                new String[]{ "name" } );

        query.setCoordinateSystem( local ); // FROM
        query.setCoordinateSystemReproject( world ); // TO

        FeatureCollection features = road.getFeatures( query );
        FeatureType resultType = features.getFeatureType();

        assertNotNull( resultType );
        assertNotSame( resultType, origionalType );

        GeometryAttributeType resultGeometryType = resultType.getDefaultGeometry();
        assertEquals( world, resultGeometryType.getCoordinateSystem() );
    }

Using a Debugger with Binary Search

This technique works like peanut butter and chocolate.

The first trick is to set a debugger break point somewhere in your code at the "half way point" and make sure you see what you expected to see.

After a bit of a wild ride (through some AbstractDataStore code) turns out that that the DefaultFeatureResults class is going to be doing most of the work.

DefaultFeatureResults Constructor

So let's make sure that we got the Query where we need it - Here is what the code looks like:

public DefaultFeatureResults(FeatureSource source, Query query) {
    this.featureSource = source;
    String typeName = source.getSchema().getTypeName();

    if( typeName.equals( query.getTypeName() ) ){
        this.query = query;
    }
    else {
        this.query = new DefaultQuery(query);
        ((DefaultQuery) this.query).setTypeName(typeName);
        ((DefaultQuery) this.query).setCoordinateSystem(query.getCoordinateSystem());
        ((DefaultQuery) this.query).setCoordinateSystemReproject(query.getCoordinateSystemReproject());

    }
}

This looks "odd" if they query type name (say "roads" equals the feature souce typeName then we use the query as is. Okay fine). The odd part is the "else" statement it should really throw an error (we are being asked to look for some content, say "rivers", that we do not have!). The code goes on to copy the CRS information we are interseted in, but does not pay attention to things like requested attributes ... oh wait it does (they Query was passed in as a constructor argument my bad).

If we want to know who did this work we can use "svn blame":

cholmesny     public DefaultFeatureResults(FeatureSource source, Query query) {
cholmesny         this.featureSource = source;
     jive         String typeName = source.getSchema().getTypeName();
  groldan
     jive         if( typeName.equals( query.getTypeName() ) ){
  groldan             this.query = query;
     jive         }
     jive         else {
    aaime             this.query = new DefaultQuery(query);
    aaime             ((DefaultQuery) this.query).setTypeName(typeName);
    aaime             ((DefaultQuery) this.query).setCoordinateSystem(query.getCoordinateSystem());
    aaime             ((DefaultQuery) this.query).setCoordinateSystemReproject(query.getCoordinateSystemReproject());
     jive         }
cholmesny     }

Looks like our friend Andrea was working on this .. lets check to see if it was needed

    public DefaultQuery(Query query) {
      this(query.getTypeName(), query.getNamespace(), query.getFilter(), query.getMaxFeatures(),
          query.getPropertyNames(), query.getHandle());
      this.sortBy = query.getSortBy();
      this.coordinateSystem = query.getCoordinateSystem();
      this.coordinateSystemReproject = query.getCoordinateSystemReproject();
    }

Looks like this is redundent; coordinateSystem and coordinateSystemReproject are getting changed twice! We can ask Andrea if he was ignoring the TypeName on purpose (or if it was mistake), While we wait to hear back lets have a look at the state of our object.

Looks like the query made it this far okay.

DefaultFeatureResults getSchema()

The first hack is about making sure the correct FeatureType is produced ... lets set a break point and see if DefaultFeatureResults.getSchema() is up to the task.

Here is what the code looks like - does not look like CRS is mentioned at all!

    public FeatureType getSchema() {
        if (query.retrieveAllProperties()) {
            return featureSource.getSchema();
        } else {
            try {
                return DataUtilities.createSubType(featureSource.getSchema(),
                    query.getPropertyNames());
            } catch (SchemaException e) {
                return featureSource.getSchema();
                //throw new DataSourceException("Could not create schema", e);
            }
        }
    }

Well we can cut and paste the working code in from DefaultQuery (remember the Hack example?). However since this work is going to be *the same* every time we run it - we may as well place the work into the consructor and have it called once.

Munching up the various examples we end up with:

public DefaultFeatureResults(FeatureSource source, Query query) {
    this.featureSource = source;        
    FeatureType origionalType = source.getSchema();
    
    String typeName = origionalType.getTypeName();        
    if( typeName.equals( query.getTypeName() ) ){
        this.query = query;
    }
    else {
        this.query = new DefaultQuery(query);
        ((DefaultQuery) this.query).setTypeName(typeName);
    }
    CoordinateReferenceSystem cs = null;        
    if (query.getCoordinateSystemReproject() != null) {
        cs = query.getCoordinateSystemReproject();
    } else if (query.getCoordinateSystem() != null) {
        cs = query.getCoordinateSystem();
    }
    try {
        if( cs == null ){
            if (query.retrieveAllProperties()) { // we can use the origionalType as is                
                schema = featureSource.getSchema();
            } else { 
                schema = DataUtilities.createSubType(featureSource.getSchema(), query.getPropertyNames());                    
            } 
        }
        else {
            // we need to change the projection of the origional type
            schema = DataUtilities.createSubType(origionalType, query.getPropertyNames(), cs, query.getTypeName(), null);
        }
    }
    catch (SchemaException e) {
        LOGGER.log( Level.WARNING, "Could not change projection to "+cs, e );
        schema = null; // client will notice something is amiss when getSchema() return null
    }
}

Not something to be proud of:

That SchemaException is being gobbled up; the origional code just "faked it" by using the origionalSchema; this would result in data being returned, with no indication that it was not as asked for!

What does the debugger say? The debugger says that the resulting schema has a "null" GeometryAttribute! Looks like our stop tomorrow will be in DataUtilities createSubType.

Main Menu

Jody Garnett