NotImpl incorrectly supporting BinaryLogicalOperator

Jody Garnett added a comment - 13/Apr/11 9:45 AM Fix applied was able to prevent the abstract base class from implementing BinaryLogicalOperator; this allows NotImpl to act correctly with instanceof checks.

Eric Sword added a comment - 13/Apr/11 11:40 AM - edited A minor point - one of the reasons I made the isGroupFilter and isLogicFilter methods is to make the checks explicitly state what I was looking for rather than how I was looking for it. Checking if (instanceof BinaryLogicalOperator || filter instanceof Not) doesn't exactly scream out "if this is a Boolean Logic filter". In fact, in most places where I use isGroupFilter, I don't really care if a filter is BinaryLogicalOperator. I care if it can contain children, so I know if I have to search it. I realize this doesn't come across in the util functions I submitted because the only uses there are when I immediately cast to BinaryLogicalOperator. Let me give an example from one of our classes. This method is called to set a new value for a filter on a particular property. (There are other methods for other props, like one for a geo property.) When the method is called, the baseFilter could be null, could be And, could be a single filter on some other property, or could be an existing filter on the PROP_DATE property. (Sorry for the lack of code formatting. Not sure why JIRA isn't giving me the option to use the "code" tags.) import org.geotools.filter.FilterUtils as fu ... Filter baseFilter ... void setTimeSpan(timeFilter) {     if (!timeFilter) {         //clear it         def oldTimeSpan = fu.findByTypeAndName(ogcFilter, PropertyIsGreaterThanOrEqualTo, PROP_DATE)         //only remove if the term is at the base level, because we will only set it at the base level         ogcFilter = fu.removeFilter(ogcFilter, oldTimeSpan, false)         return     }     //shortcut to see if the date prop is used at all. If not, then can simply add the new one to it     if (!fu.uses(ogcFilter, PROP_DATE)) {         ogcFilter = fu.addFilter(ogcFilter, timeFilter)         return     }     //we know the time prop is used somewhere, so replace wherever it is. We can't check for a specific     //filter type because it could be in a GreaterThan or a Between (when we support that in the future)     if (fu.isGroupFilter(ogcFilter)) {         def oldTimeFilters = ogcFilter.accept(new FindProperty(PROP_DATE), [])         //just do the first one         ogcFilter.accept(new Replace(oldTimeFilters[0], timeFilter), null)     } else {         //its just a single filter and we know it uses the time prop, so replace it         ogcFilter = timeFilter     } } I was also trying to future-proof the code so that if there was ever any other type of Filter that could contain children, I didn't have to find all the places where I made "(x instanceof BinaryLogicalOperator)" checks. But I realize that is unlikely given that the OGC Filter specs would have to change for such a thing.

Jody Garnett added a comment - 13/Apr/11 7:02 PM Yeah I know this fix was not what you intended; your fix pointed out that casting to BinaryLogicalOperator was not a safe assumption. ie we screwed up our implementation and your code was paying the price. For an alternative to isGroupFilter how about this: - Filters.hasChildren( filter ) - Filters.children( filter ) you should be good? Comment: In you example you are doing multiple walks of the filter data structure (example fu.uses(ogcFilter, PROP_DATE)), I would of tried extended DuplicatingFIlterVisitor (since I know I am doing a transform of some sort) and written a single visitor for this example. Aside: Jira only lets you do code formatting in the problem description

Andrea Aime added a comment - 28/May/11 4:19 AM Mass closing all issues that have been in "resolved" state for more than one month without further comments