GeoTools

Filter manipulation Vistors and utilities contribution

Details

  • Type: New Feature New Feature
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: 2.7.0
  • Fix Version/s: None
  • Component/s: main
  • Labels:
    None
  • Testcase included:
    yes

Description

Per discussion on this thread (http://osgeo-org.1803224.n2.nabble.com/Contribution-of-some-FilterVistors-and-other-utilities-tp6265810p6265810.html), attached are some FilterVisitors and utility functions for manipulating filters.

The FilterTestContext contains a bit of extra code because we use it for classes that I am not contributing (yet). I can only translate so much groovy to java at a time. Putting in all the semicolons and typecasting drives one bonkers after a while.

Activity

Ascending order - Click to sort in descending order
Jody Garnett made changes -
Field Original Value New Value
Comment [ Okay reviewing now:

    static boolean isLogicFilter(Filter filter) {
        return (isGroupFilter(filter) || (filter instanceof Not));
    }
    static boolean isGroupFilter(Filter filter) {
        //Note: Can't use BinaryLogicOperator here because the Not implementation also inherits from it.
        return ( (filter instanceof And) || (filter instanceof Or));
    }

These show a really tough problem where code reuse in our implementation wreck what should be a simple instance of check.

PROBLEM:

BinaryLogicalOperator > BinaryLogicalAbstract > LogicFilterImpl > (AndImpl, NotImpl, OrImpl)

SOLUTION:

Make BinaryLogicalAbstract not implement BinaryLogicalOperator (replying on AndIml and OrIml to get BinraryLogicalOperator via their interface only).

Trying out the solution now.

UDATE:

This worked out fine; the above two methods are no longer needed the result can now be accomplished cleanly using instance of checks.

a) filter instanceof BinaryLogicalOperator || filter instanceof Not
b) filter instance BinaryLogicalOperator
]
Jody Garnett made changes -
Comment [ Third Feedback:

We seem to have missed the point with the Filter classes; the contents of an And or Or filter is not a list that should be changed; I better change the implementation to protect against this. We rely on filters being immutable once constructed for thread safety.

- Change And / Or to pass out an unmodifiable list
- Update implementations to make a copy, modify, and then return a modified filter

Q: Did you look at the existing duplication filter visitor? ]
Jody Garnett made changes -
Comment [ Second feedback on a lot of the utility methods:

- they are static but not public
- they are static and refer to a static FilterFactory

Option A) make them public and non static - code example:

   Filters filters = new Filters( filterFactory ); // null would use the global default
   filters.addFilter( baseFilter, newFilter )

Option B) have each method use CommonFactoryFinder

Option C) leave well enough alone and give up on people using their own FilterFactory? Oh wait the application schema people do use custom filter factories in order to throw exceptions when a propertyName is invalid

  optiowe try not to rely on a static copy of FilterFactory: the Filters utility can be constructed with a FilterFactory of your choice) ]
Jody Garnett made changes -
Comment [ This one is just a general feedback on visitor names; I know this is no fault of your own....

There is a much loved/hated class called PostPreProcessFilterSplittingVisitor ... your new abstract base class PrePostFilterVisitor is close enough in name that I am always going to be confused.

I am going to guess that you use this base class for more than just the search (and replace) examples provided? If not I would love to call this AbstractSearchFilterVisitor; and then the other classes would cleanly follow from it...

Other than that I gotta say that this preVisit( object, data ) and postVisit( object, data ) is exactly what the visitor pattern is trying to avoid (ie the implementations are performing instance of checks in here to try and recover the type information you just threw out.

I am going to think a bit more about what you have here and see if I can help; it looks like you were trying to duck out of implementing filter traversal while still being able to subclass quickly and "process" the data structure.

This is supposed to be possible with DefaultFilterVisitor; it may be a design or documentation flaw if you did not find this usable. ]
Jody Garnett made changes -
Component/s main [ 10810 ]
Component/s core filter [ 11601 ]

People

  • Assignee:
    Unassigned
    Reporter:
    Eric Sword
Vote (0)
Watch (0)

Dates

  • Created:
    Updated: