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
>>>>>> Geotools-devel@...
>>>>>> 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
>>> Geotools-devel@...
>>> 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
>> Geotools-devel@...
>> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>>
>
>
>
|