GeoTools
  1. GeoTools
  2. GEOT-3569

Add detailed information about Function arguments and return type to FunctionName

    Details

    • Type: Improvement Improvement
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 8.0
    • Component/s: main
    • Labels:
      None
    1. GEOT-3569.patch
      33 kB
      Justin Deoliveira
    2. GEOT-3569.patch
      20 kB
      Justin Deoliveira
    3. GEOT-3569-2.patch
      5 kB
      Justin Deoliveira
    4. GEOT-3569-3.patch
      6 kB
      Justin Deoliveira

      Activity

      Hide
      Jody Garnett added a comment -
      Okay Ben and myself stalled out trying to hit the implementation side of things with respect to validation:

      - factory creation of function name should not be subject to any restrictions we have on the GeoTools side of things (as we need the data structure to communicate the capabilities of external services and sql functions etc...)
      - existing symbology encoding functions support several parameters with multiplicity (not just the last one)
      - existing app schema functions support several optional parameters (not just the last one)

      With that in mind I would like to remove the validation check; if a function implementor wants to take on the pain of sorting out things they should be able to do so. Note this does not require the introduction of named parameters or any change to the FunctionFinder (lookup is on function name only).

      Options:
      a) just remove the validation check
      b) remove the validation check from the factory and the direct FunctionNameImpl calls; leaving it in the utility method as a way to encourage function implementors from taking on too much work
      Show
      Jody Garnett added a comment - Okay Ben and myself stalled out trying to hit the implementation side of things with respect to validation: - factory creation of function name should not be subject to any restrictions we have on the GeoTools side of things (as we need the data structure to communicate the capabilities of external services and sql functions etc...) - existing symbology encoding functions support several parameters with multiplicity (not just the last one) - existing app schema functions support several optional parameters (not just the last one) With that in mind I would like to remove the validation check; if a function implementor wants to take on the pain of sorting out things they should be able to do so. Note this does not require the introduction of named parameters or any change to the FunctionFinder (lookup is on function name only). Options: a) just remove the validation check b) remove the validation check from the factory and the direct FunctionNameImpl calls; leaving it in the utility method as a way to encourage function implementors from taking on too much work
      Hide
      Justin Deoliveira added a comment -
      Ok, how about this patch. Basically removed the validate() call from the FUnctionNameImpl constructor and move it to FunctionImpl.dispatchArguments(), which is a new method that functions can call to dispatch arguments according to the java convention for variable arguments. No functions currently call that method (except for one test function) so it shouldn't break anything. So basically functions are responsible on their own for handling function arguments, unless they call the method in which they are forced to follow the conventions.
      Show
      Justin Deoliveira added a comment - Ok, how about this patch. Basically removed the validate() call from the FUnctionNameImpl constructor and move it to FunctionImpl.dispatchArguments(), which is a new method that functions can call to dispatch arguments according to the java convention for variable arguments. No functions currently call that method (except for one test function) so it shouldn't break anything. So basically functions are responsible on their own for handling function arguments, unless they call the method in which they are forced to follow the conventions.
      Hide
      Jody Garnett added a comment -
      1) the patch looks good and should allow us to proceed (thanks!)

      2) The validate() method could describe that it throws IllegalArgumentException in the javadocs (or even explicitly with a throws IllegalArgumentException (I had to read the code to sort out why it was not returning "true or false". I agree that throwing an exception is nicer as it gives the caller an actual error message.

      private void validate() throws IllegalArgumentException {
         ...
      }

      3) Now understand your comment about dispatch (from the proposal we did not know about prepArguments producing a Map<String,Object>. I think function implementors will like this one.
          
      Reading the code I wonder why you call evaualte( feature ) - this amounts to evaulate(feature, Object) and does not allow the functions to perform their own coversion as per the API contract. This is functionality I have dependend on in the use in order to allow a string to be converted to an enumerated type for example).
          
      So I would update the code to have:
          
          Object o = expr.get(i).evaluate(obj, parameter.getType() );

      I understand that Andrea is tired of the filter / expression system being forgiving and would like to see real error messages. The conversation deserves its own Jira; but this change does finally enable us to provide real error messages.
          
          Object o = expr.get(i).evaualte( obj, parameter.getType() );
          if( o == null ){
                // check if it was a conversion error
                o = expr.evaulate( obj );
                if( o != null ){
                       throw new IllegalArgumentException( "Function "+name+" requires argument "+parameter.getName()+" to be "+parameter.getType()+". Unable to use:"+expr+" with value: "+o"");
                }
          }

      Show
      Jody Garnett added a comment - 1) the patch looks good and should allow us to proceed (thanks!) 2) The validate() method could describe that it throws IllegalArgumentException in the javadocs (or even explicitly with a throws IllegalArgumentException (I had to read the code to sort out why it was not returning "true or false". I agree that throwing an exception is nicer as it gives the caller an actual error message. private void validate() throws IllegalArgumentException {    ... } 3) Now understand your comment about dispatch (from the proposal we did not know about prepArguments producing a Map<String,Object>. I think function implementors will like this one.      Reading the code I wonder why you call evaualte( feature ) - this amounts to evaulate(feature, Object) and does not allow the functions to perform their own coversion as per the API contract. This is functionality I have dependend on in the use in order to allow a string to be converted to an enumerated type for example).      So I would update the code to have:          Object o = expr.get(i).evaluate(obj, parameter.getType() ); I understand that Andrea is tired of the filter / expression system being forgiving and would like to see real error messages. The conversation deserves its own Jira; but this change does finally enable us to provide real error messages.          Object o = expr.get(i).evaualte( obj, parameter.getType() );     if( o == null ){           // check if it was a conversion error           o = expr.evaulate( obj );           if( o != null ){                  throw new IllegalArgumentException( "Function "+name+" requires argument "+parameter.getName()+" to be "+parameter.getType()+". Unable to use:"+expr+" with value: "+o"");           }     }
      Hide
      Justin Deoliveira added a comment -
      Thanks for the review Jody. This patch should address all the feedback.
      Show
      Justin Deoliveira added a comment - Thanks for the review Jody. This patch should address all the feedback.
      Hide
      Andrea Aime added a comment -
      Seems all good to me too
      Show
      Andrea Aime added a comment - Seems all good to me too
      Hide
      Justin Deoliveira added a comment -
      Patch committed.
      Show
      Justin Deoliveira added a comment - Patch committed.
      Hide
      Jody Garnett added a comment -
      I would like to keep this open until we are done the work for the proposal (no half measures etc..).

      I have had some good luck using the following approach:

      import static org.geotools.filter.capability.FunctionNameImpl.parameter;
      ...
          public static FunctionName NAME = new FunctionNameImpl("rint",
                  parameter("related", Boolean.class),
                  parameter("geometry", Geometry.class),
                  parameter("geometry", Geometry.class),
                  parameter("pattern", String.class));
      Show
      Jody Garnett added a comment - I would like to keep this open until we are done the work for the proposal (no half measures etc..). I have had some good luck using the following approach: import static org.geotools.filter.capability.FunctionNameImpl.parameter; ...     public static FunctionName NAME = new FunctionNameImpl("rint",             parameter("related", Boolean.class),             parameter("geometry", Geometry.class),             parameter("geometry", Geometry.class),             parameter("pattern", String.class));
      Hide
      Andrea Aime added a comment -
      Aren't we done already?
      Show
      Andrea Aime added a comment - Aren't we done already?
      Hide
      Jody Garnett added a comment -
      Show
      Jody Garnett added a comment - I think we may be. I have slotted http://docs.codehaus.org/display/GEOTOOLS/Detailed+Argument+and+Return+Info+for+FunctionName in under 8.x.

        People

        • Assignee:
          Jody Garnett
          Reporter:
          Justin Deoliveira
        • Votes:
          0 Vote for this issue
          Watchers:
          2 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved: