[Geoserver-devel] Filter encoding issues

Hi all,

Some recent development on geotools 2.2.x has caused filter encoding to
go bad and as a result is making geoserver cite tests fail, which means
we cant release.

After three hours of bug hunting i have found the following bugs:

1. FilterCapabilties being wiped out by a negative bit mask
(Filter.ALL), so datastores can receive "pre" filters to encode that
they cannot handle.
2. The filter splitter changes the filter of an incoming Query object to
only be the preFilter. This is a bad assumption to make that the
datastore can change the query object passed into it, this makes it so
it cant be reused by the calling application.

The pre vs post filter processing (used to be SQLUnpacker) appears to
have been totally rewritten. This is annoying for two reasons:

1. The postgis module maintainers (chris and I) were not consulted
before hand.

2. 2.2.x is supposed to be "stable" which means just bug fixes. This
goes far beyond that.

I am not trying to point fingers, I am as much to blame as anyone as I
have not been keeping up with what is going on apparently.

I cant really spend much more time hunting these bugs down. Also since
its radically different i am not confident I will make the appropriate
changes properly. So I am in a position where my only option is to roll
geoserver back to an earlier 2.2.x version.

-Justin

--
Justin Deoliveira
The Open Planning Project
jdeolive@anonymised.com

Justin Deoliveira wrote:

Hi all,

Some recent development on geotools 2.2.x has caused filter encoding to
go bad and as a result is making geoserver cite tests fail, which means
we cant release.

After three hours of bug hunting i have found the following bugs:

1. FilterCapabilties being wiped out by a negative bit mask
(Filter.ALL), so datastores can receive "pre" filters to encode that
they cannot handle.
2. The filter splitter changes the filter of an incoming Query object to
only be the preFilter. This is a bad assumption to make that the
datastore can change the query object passed into it, this makes it so
it cant be reused by the calling application.

The pre vs post filter processing (used to be SQLUnpacker) appears to
have been totally rewritten. This is annoying for two reasons:

1. The postgis module maintainers (chris and I) were not consulted
before hand.

2. 2.2.x is supposed to be "stable" which means just bug fixes. This
goes far beyond that.

I am not trying to point fingers, I am as much to blame as anyone as I
have not been keeping up with what is going on apparently.

Well, I'm to blame as well, since though I wasn't initially consulted on the change, I insisted that if the change was to be done it be done right, which was more substantial work on a stable branch. Probably should have insisted that the new way of doing things was just a switch that wasn't on by default. That may be the best way forward? I think it may be in just DefaultSQLBuilder and some small other place where it makes the split (or that may be actually be done in the sqlbuilder).

I'm also a bit scared of the GEOS and non-GEOS postgis merge. Not because it doesn't need to be done, but because it has the possibility of introducing some hard to find bugs. 'nice to have' changes on things that are already working well and tested should _not_ be done right before we're about to make a .0 release. And I'd put the filter capabilities stuff in the same category - we had a stable way of doing things, if uDig requires a major change then they should be on the next version. And if we actually are able to get the api to calm down we could theoretically just use a 2.3 jar for postgis with 2.2 for the rest.

Chris

I cant really spend much more time hunting these bugs down. Also since
its radically different i am not confident I will make the appropriate
changes properly. So I am in a position where my only option is to roll
geoserver back to an earlier 2.2.x version.

-Justin

--
Chris Holmes
The Open Planning Project
http://topp.openplans.org

Chris Holmes wrote:

Justin Deoliveira wrote:

Some recent development on geotools 2.2.x has caused filter encoding to
go bad...

The pre vs post filter processing (used to be SQLUnpacker) appears to
have been totally rewritten. This is annoying for two reasons:

1. The postgis module maintainers (chris and I) were not consulted
before hand.

Please add yourself to the pom. Some consultation took place, but certainly not enough.

2. 2.2.x is supposed to be "stable" which means just bug fixes. This
goes far beyond that.

I am not trying to point fingers, I am as much to blame as anyone as I
have not been keeping up with what is going on apparently.

My apologies -- I am to blame for this, but these changes are important (otherwise we are shipping with some significant flaws -- ie "stable", but broken). Some changes were a little beyond just a bug fix... so three lashings with the pointy stick for me.

I'm also a bit scared of the GEOS and non-GEOS postgis merge. Not because it doesn't need to be done, but because it has the possibility of introducing some hard to find bugs. 'nice to have' changes on things that are already working well and tested should _not_ be done right before we're about to make a .0 release. And I'd put the filter capabilities stuff in the same category - we had a stable way of doing things, if uDig requires a major change then they should be on the next version. And if we actually are able to get the api to calm down we could theoretically just use a 2.3 jar for postgis with 2.2 for the rest.

Fair enough. A non-GEOS PostGIS still sounds pretty obscure to me -- the split was quite confusing. I don't think this is risky since a non-GEOS PostGIS only supports SPATIAL_BBOX... so the code remains pretty much the same except it short circuits if there is no GEOS support.

Thanks for whacking those bugs Justin :slight_smile:

Cory.