Stefan, Don't worry about the patch for now. Doing the WFS datastore with the 'new style' FilterCapabilities is a good idea, and I don't think there's any pressing need from my end to get it done right away. There'll be a release soon, so if you want to get your fix in to geoserver 1.6.0, then you should get it done before they release that.
Otherwise take your time! --saul On Wed, 2007-10-31 at 15:01 +1100, Stefan Hansen wrote: > Hey Ho! > > I'm happy to create a patch for the (1). As far as (2) is concerned, it > doesn't make any sense to use my solution if it uses a "deprecated > style". I will look into the code of the WFS datastore (I discovered the > bugs by using this datastore) and see if I can find a solution > supporting the "new style". > > Unfortunately, I have heaps of things to do in the next couple of days. > So I don't think, I will be able to finish any of the things I just > promised to do before the end of next week. I hope you can wait that > long. Otherwise someone else has to volunteer. > > cheers, > stefan > > > Saul Farber wrote: > > Ahh, the curse of the "willing to do it". > > > > Stefan, I've looked at your suggestions, and here are my thoughts: > > > > 1) I'm not sure I understand the changes well enough on this one, but > > if you could provide a patch using "svn diff" on the PPPFSV.java file, > > then I can review and give you the ok on this one. > > > > > > 2) You've hit a recurring problem with PPPFSV. FilterCapabilities have > > two 'styles'. One is the "old style" with > > FilterCapabilities.INTEGER_CONSTANT as the definition of the capability. > > The other is the "new style" with Capability.class as the definition of > > the capability. Your patch which includes BOTH checks looks good. > > Leaving one off will work for you, but break other datastores that use > > the new filter capabilities style exclusively. Also, as andrea pointed > > out, you should have your FilterCapabilities declared in the "new style" > > as well (if not exclusively). > > > > > > For both patches, you'll need to do the following: > > > > a. Make your changes to your local subversion working copy. > > b. Run '$ mvn clean install to make sure all tests are still passing. > > c. create an "svn diff" of your patch and send it to the list for > > review (attaching it to the JIRA issue would be just as good, if not > > better) > > d. Port your patch FORWARD to geotools-trunk. the PPPFSV class hasn't > > changed much between the two branches, so probably just 'svn diff' and > > 'patch -p0 < your-patch-file.patch' should do it. If not, eclipse and > > 'compare with each other...' will do you wonders. > > e. Make sure that the patched geotools-trunk also is passing all tests > > (or at least those tests which you probably didn't break) > > f. Commit! > > > > > > If you don't have the time/interest in doing steps a-e, please don't do > > step f...but please *do* create patches and then I can do all 6 steps > > just to make sure 2.4.x and trunk get the benefit of the patches (and > > stay stable). > > > > If you're really ambition, before step a make your own test-case that > > tests exactly the bug you're fixing, and add it to the appropriate > > testing classes. That way this bug won't ever come back (as long as > > people do all 6 of the above steps!) > > > > > > Thanks much for finding these bugs and taking the time to report them in > > such detail to the list! We really appreciate it. > > > > Go Sox, > > --saul > > > > > > On Sun, 2007-10-28 at 22:59 -0700, Justin Deoliveira wrote: > > > >> Also it is always nice to have the patch in a diff format that can be > >> readily applied. As for the patches they look reasonable to me. I > >> believe Saul has become the defacto maintainer for the filter > >> capabilities stuff on main so you may want to get his review before > >> committing. > >> > >> -Justin > >> > >> Cameron Shorter wrote: > >> > >>> Stefan, > >>> For these 2 issues you have found, I suggest you create an issue in the > >>> Geoserver issue tracker. > >>> I understand that you have fixed these issues in your branch. I suggest > >>> you reference the svn version you committed which makes it easier to > >>> review. > >>> Be specific about what you would like, and how you can help. In this > >>> case, ask if it is ok if you can commit this patch back to the trunk, or > >>> if someone else can do it for you. > >>> > >>> Geotools, > >>> I suggest requesting codehaus track geotools with > >>> http://fisheye.codehaus.org/ > >>> It provides lots of useful metics and web based svn diffs. > >>> > >>> Stefan Hansen wrote: > >>> > >>>> Dear list! > >>>> > >>>> I discovered a small problem in the function visit(GeometryFilter > >>>> filter) of > >>>> org.geotools.filter.visitor.PostPreProcessFilterSplittingVisitor.java. > >>>> > >>>> If the given filter is a FilterType.GEOMETRY_BBOX containing a BBOX that > >>>> lies completely outside the maxbox, the BBOX in the filter gets altered. > >>>> Because of this, sometimes features are returned, that are not in the > >>>> requested BBOX. > >>>> > >>>> Here is the code snippet of which I think causes the problem: > >>>> > >>>> if(bbox!=null){ > >>>> boolean changed = false; > >>>> double minx,miny,maxx,maxy; > >>>> minx = bbox.getMinX(); > >>>> miny = bbox.getMinY(); > >>>> maxx = bbox.getMaxX(); > >>>> maxy = bbox.getMaxY(); > >>>> if(minx < maxbbox.getMinX()){ > >>>> minx = maxbbox.getMinX(); > >>>> changed = true; > >>>> } > >>>> if(maxx > maxbbox.getMaxX()){ > >>>> maxx = maxbbox.getMaxX(); > >>>> changed = true; > >>>> } > >>>> if(miny < maxbbox.getMinY()){ > >>>> miny = maxbbox.getMinY(); > >>>> changed = true; > >>>> } > >>>> if(maxy > maxbbox.getMaxY()){ > >>>> maxy = maxbbox.getMaxY(); > >>>> changed = true; > >>>> } > >>>> if(changed){ > >>>> Envelope tmp = new > >>>> Envelope(minx,maxx,miny,maxy); > >>>> try { > >>>> le.setLiteral((new > >>>> GeometryFactory()).toGeometry(tmp)); > >>>> } catch (IllegalFilterException > >>>> e) { > >>>> logger.warning(e.toString()); > >>>> } > >>>> } > >>>> > >>>> And here is my solution, which is supposed to alter the BBOX only if it > >>>> overlaps the maxbox: > >>>> > >>>> if(bbox!=null){ > >>>> logger.warning("sh: GeometryFilter: > >>>> BBOX!=null"); > >>>> boolean changed = false; > >>>> double minx,miny,maxx,maxy; > >>>> minx = bbox.getMinX(); > >>>> miny = bbox.getMinY(); > >>>> maxx = bbox.getMaxX(); > >>>> maxy = bbox.getMaxY(); > >>>> if(minx < maxbbox.getMinX() && maxx > >>>> > maxbbox.getMinX()){ > >>>> minx = maxbbox.getMinX(); > >>>> changed = true; > >>>> } > >>>> if(maxx > maxbbox.getMaxX() && minx > >>>> < maxbbox.getMaxX()){ > >>>> maxx = maxbbox.getMaxX(); > >>>> changed = true; > >>>> } > >>>> if(miny < maxbbox.getMinY() && maxy > >>>> > maxbbox.getMinY()){ > >>>> miny = maxbbox.getMinY(); > >>>> changed = true; > >>>> } > >>>> if(maxy > maxbbox.getMaxY() && miny > >>>> > maxbbox.getMaxY()){ > >>>> maxy = maxbbox.getMaxY(); > >>>> changed = true; > >>>> } > >>>> if(changed){ > >>>> logger.warning("sh: > >>>> GeometryFilter: Changed true"); > >>>> Envelope tmp = new > >>>> Envelope(minx,maxx,miny,maxy); > >>>> try { > >>>> le.setLiteral((new > >>>> GeometryFactory()).toGeometry(tmp)); > >>>> } catch (IllegalFilterException > >>>> e) { > >>>> logger.warning(e.toString()); > >>>> } > >>>> } > >>>> > >>>> cheers, > >>>> stefan > >>>> > >>>> ------------------------------------------------------------------------- > >>>> This SF.net email is sponsored by: Splunk Inc. > >>>> Still grepping through log files to find problems? Stop. > >>>> Now Search log events and configuration files using AJAX and a browser. > >>>> Download your FREE copy of Splunk now >> http://get.splunk.com/ > >>>> _______________________________________________ > >>>> Geotools-devel mailing list > >>>> [email protected] > >>>> https://lists.sourceforge.net/lists/listinfo/geotools-devel > >>>> > >>>> > >>>> > >>> > >> > > > > > > ------------------------------------------------------------------------- > > This SF.net email is sponsored by: Splunk Inc. > > Still grepping through log files to find problems? Stop. > > Now Search log events and configuration files using AJAX and a browser. > > Download your FREE copy of Splunk now >> http://get.splunk.com/ > > _______________________________________________ > > Geotools-devel mailing list > > [email protected] > > https://lists.sourceforge.net/lists/listinfo/geotools-devel > > > > > > > > > ------------------------------------------------------------------------- > This SF.net email is sponsored by: Splunk Inc. > Still grepping through log files to find problems? Stop. > Now Search log events and configuration files using AJAX and a browser. > Download your FREE copy of Splunk now >> http://get.splunk.com/ > _______________________________________________ > Geotools-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/geotools-devel ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/ _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
