Hello all, I've chatted with a few of you over the last week or two about moving some of the internals of geotools from "old" (org.geotools.filter) filter usage to "new" (org.opengis.filter) filter usage.
In particular, I was itching to get the ArcSDE datastore plugin moved completely off of the "old" filters and completely onto the "new" filters. Most of the ArcSDE datastore plugin moved quite cleanly over, but there were a few bits of main and jdbc that were being pulled in, and which were using deprecated interfaces. So I set about to: 1) Clean up SQLEncoder.java, moving it from using "old" filters to "new" filters 2) Stop using the SQLUnpacker class and instead use PostPreFilterSplittingVisitor for the SDE internal filter splitting needs In order to complete this work I had to make two non-trivial decisions that I'd like to run by any interested parties (particularly the main and jdbc module maintainers: jody and cory!) Currently all tests pass and I'm synced to the latest svn revision with no conflicts. 1) Trying to clean up SQLEncoder.java turned out to be a bad idea. There are many datastore plugins which subclass this class as a way of getting easy filter->SQL encoding support, plus a few customizations. Changing the underlying methods of SQLEncoder was no good as I don't really have the expertise to fix all the datastores at the same time. So instead I created a *new* port of the class (called 'GeoAPIFilterToSQLEncoder') in the org.geotools.data.jdbc package. This new class is really just a port of the old SQLEncoder class, but made to use "new" filters. I then deprecated the old org.geotools.filter.SQLEncoder class (and it's exceptions) and ported its tests/exceptions over to the new GeoAPIFilterToSQLEncoder. I left a note in SQLEncoder about which class replaces this now-deprecated class. QUESTION: What should the name of the new SQLEncoder class be? Is the new *different* name for the class in a different package a good one? Or should it keep the same name, just change package? QUESTION: What should the *package* be of the new SQLEncoder class (regardless of its name)? The old SQLEncoder lived in org.geotools.filter. SHould the new one live in org.opengis.filter? Or is living in org.geotools.data.jdbc good enough/better? WRAPUP: After creating and testing this new class, I then ported the SQLEncoderSDE over to use the new class, and everything works great with 100% "new" filters. 2) Went fine. Moving from SQLUnpacker to PostPreProcessFilterSplittingVisitor is fairly painless, once you've figured out how PPPFSV works. One comment: FilterCapabilities now represents two things. It represents the capabilites of a filter as expressed with *old* filters, and the capabilities of a filter as expressed with *new* filters. This means that the FilterCapabilities class is a bit of a frankenstein, with code handling each individual case/bit in each method and use-case. I would suggest that either: a. This is accepted as an ok thing, and the class is re-architected to account for that. b. This is rejected as a *bad* thing, and the class is split into an older, deprecated version and a newer version called "FilterCapabilitiesImpl" which implements the org.opengis.filter.capabilities.FilterCapabilities interface (or whatever the right interface is in geoapi). Ok, here's a list of files changed and the nature of the changes: deprecation: Index: modules/library/jdbc/src/main/java/org/geotools/filter/SQLEncoder.java Index: modules/library/jdbc/src/main/java/org/geotools/filter/SQLEncoderException.java Change to support "new" filters rather than "old" filters Index: modules/library/main/src/main/java/org/geotools/filter/visitor/PostPreProcessFilterSplittingVisitor.java Minor changes/cleanup to support the new PostPreProcessFilterSplittingVisitor: Index: modules/library/jdbc/src/main/java/org/geotools/data/jdbc/DefaultSQLBuilder.java Index: modules/library/main/src/test/java/org/geotools/filter/visitor/PostPreProcessFilterSplittingVisitorTest.java Index: modules/library/main/src/test/java/org/geotools/filter/visitor/AbstractPostPreProcessFilterSplittingVisitorTests.java Index: modules/library/main/src/test/java/org/geotools/filter/visitor/PostPreProcessFilterSplittingVisitorSpatialTest.java Index: modules/library/main/src/test/java/org/geotools/filter/visitor/PostPreProcessFilterSplitterVisitorFunctionTest.java Index: modules/plugin/wfs/src/main/java/org/geotools/data/wfs/WFSDataStore.java More substantial changes were required to get FilterCapabilities to work as a representation of *both* new and old FilterCapabilities. Index: modules/library/main/src/main/java/org/geotools/filter/FilterCapabilities.java New files: modules/library/jdbc/src/main/java/org/geotools/data/jdbc/GeoAPIFilterToSQLEncoderException.java modules/library/jdbc/src/main/java/org/geotools/data/jdbc/GeoAPIFilterToSQLEncoder.java modules/library/jdbc/src/test/java/org/geotools/data/jdbc/SQLFilterSuite.java modules/library/jdbc/src/test/java/org/geotools/data/jdbc/SQLFilterTestSupport.java modules/library/jdbc/src/test/java/org/geotools/data/jdbc/GeoAPIFilterToSQLEncoderTest.java Big question: Jody/cory/whoever. Should I commit this? Does this qualify as a GTIP/big-change that needs voting or more formal documentation? Do you want to see a copy of it to review it? I'm committed to tracking down any code that uses the old SQLUnpacker or SQLEncoder classes and either: a. Bugging the maintainer to change it b. Changing it myself it it's not maintained (in unsupported) Not sure if that changes the answer to the above question. Sorry for the long one! --saul ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
