Hi! I just attached diff-files for 2.4.x and trunk to issue GEOT-1549 (the problem with altered bbox in visit(GeometryFilter filter) of org.geotools.filter.visitor.PostPreProcessFilterSplittingVisitor.java). Maybe someone could review that?
I also had a look into the other problem (GEOT-1550 - old style filters). I'm probably not the best person to fix this, since I don't too much about Geotools and I'm not able to spent too much time on this at the moment :'(. However, I think the problem is actually in org.geotools.filter.FilterNameTypeMapping, which is used by the WFS datastore, but only uses the "old style filters". I tried to change it a bit, so it uses both styles and for the few tests I ran it seems to work. But I don't think it is complete (i.e, covering all filters). I attached a diff-file of my changes to the issue, maybe it is a little help. cheers, stefan Saul Farber wrote: > 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
