Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    1. move_common_factory_finder.patch
      52 kB
      Jody Garnett
    2. move_common_factory_finder.patch
      3 kB
      Jody Garnett
    3. namespace.patch
      30 kB
      Jody Garnett
    4. namespace.patch
      13 kB
      Jody Garnett
    5. newpatch_namespaces.diff
      29 kB
      Niels Charlier
    6. patch_namespacesupport.diff
      23 kB
      Niels Charlier

      Issue Links

        Activity

        Hide
        Jody Garnett added a comment -
        So one of the issues was Query not being able to see CommonFactory finder. I have prepared a patch to move CommonFactory finder to gt-api. I actually like the result there a lot better; leaving gt-main to be strictly implementation.

        There were two areas of difficulty in moving CommonFactoryFinder:
        - Some old references to FunctionImpl in CommonFactoryFinder
        - Explicit reference to LienientFeatureFactoryImpl
        Show
        Jody Garnett added a comment - So one of the issues was Query not being able to see CommonFactory finder. I have prepared a patch to move CommonFactory finder to gt-api. I actually like the result there a lot better; leaving gt-main to be strictly implementation. There were two areas of difficulty in moving CommonFactoryFinder: - Some old references to FunctionImpl in CommonFactoryFinder - Explicit reference to LienientFeatureFactoryImpl
        Hide
        Jody Garnett added a comment -
        Patch reviewed (thanks justin) and now applied to trunk.

        Ball back in your court Niels.
        Show
        Jody Garnett added a comment - Patch reviewed (thanks justin) and now applied to trunk. Ball back in your court Niels.
        Hide
        Niels Charlier added a comment -
        Okay, here is the patch. Some remarks:

        - In Query, the old methods are called get-/setPropertyNames (with String[]), the new ones are called get-/setProperties (with List<PropertyName>). There were special constants defined to use for the old methods called NO_NAMES and ALL_NAMES, I added new constants called NO_PROPERTIES and ALL_PROPERTIES to fulfill the same role for the new methods.

        - Previously there was a hint key for NamespaceSupport defined in the XpathPropertyAccessorFactory and another hint key for the same purpose in FeaturePropertyAccessorFactory. To avoid confusion and error (there should be one and only hint key as they will not be recognized as equal!), I removed both and created a new one in the PropertyAccessorFactory interface (the advantage being that any existing references to the old ones will still work).

        - The FilterFactoryImplNamespaceAware in app-schema has not been removed, but updated to be built on top of the existing namespace support in FF. The reason being that it works with one single NamespaceSupport object for the whole filter factory and this ff is passed on to other classes and it would take some work to change that. This can be optionally done within app-schema at a later stage, but it's not really necessary.

        - I needed to make a small change to a class in the 'feature-pregeneralized' package to adapt to the change in the PropertyName interface.e
        Show
        Niels Charlier added a comment - Okay, here is the patch. Some remarks: - In Query, the old methods are called get-/setPropertyNames (with String[]), the new ones are called get-/setProperties (with List<PropertyName>). There were special constants defined to use for the old methods called NO_NAMES and ALL_NAMES, I added new constants called NO_PROPERTIES and ALL_PROPERTIES to fulfill the same role for the new methods. - Previously there was a hint key for NamespaceSupport defined in the XpathPropertyAccessorFactory and another hint key for the same purpose in FeaturePropertyAccessorFactory. To avoid confusion and error (there should be one and only hint key as they will not be recognized as equal!), I removed both and created a new one in the PropertyAccessorFactory interface (the advantage being that any existing references to the old ones will still work). - The FilterFactoryImplNamespaceAware in app-schema has not been removed, but updated to be built on top of the existing namespace support in FF. The reason being that it works with one single NamespaceSupport object for the whole filter factory and this ff is passed on to other classes and it would take some work to change that. This can be optionally done within app-schema at a later stage, but it's not really necessary. - I needed to make a small change to a class in the 'feature-pregeneralized' package to adapt to the change in the PropertyName interface.e
        Hide
        Jody Garnett added a comment -
        reviewed patch; added javadocs and a few null checks in the implementation of Query; seems good
        Show
        Jody Garnett added a comment - reviewed patch; added javadocs and a few null checks in the implementation of Query; seems good
        Hide
        Niels Charlier added a comment -
        Jody, your changes look good, but just one comment. You did this:

        in FilterFactoryImpl:

             public PropertyName property( Name name ) {
        - return property( name.toString() ); // uses full URI for justin
        + return new AttributeExpressionImpl( name );
        + }

        in AttributeExpressionImpl:

        + public AttributeExpressionImpl( Name name ){
        + this( name.getLocalPart() ); // uses full URI for justin
        + }



        Note that you are using only the Local Part rather than the full string with namespace included.
        I thought we trying to move away from throwing namespaces in the bin...

        However, this reminds me of another issue I haven't been able to address yet. Attribute Expressions of the form
        "full_namespace:property_name" currently cannot be evaluated by the feature property accessors because X-path doesn't accept full namespaces. Perhaps to solve this issue we can use the NamespaceSupport object and the concept of the default namespace? Something like this:

        public AttributeExpressionImpl( Name name ){
                attPath = name.getLocalPart();
                schema = null;
                this.namespaceSupport = new NamespaceSupport();
                namespaceSupport.addPrefix("", name.getNamespaceURI());
                this.expressionType = ATTRIBUTE;
          }
        Show
        Niels Charlier added a comment - Jody, your changes look good, but just one comment. You did this: in FilterFactoryImpl:      public PropertyName property( Name name ) { - return property( name.toString() ); // uses full URI for justin + return new AttributeExpressionImpl( name ); + } in AttributeExpressionImpl: + public AttributeExpressionImpl( Name name ){ + this( name.getLocalPart() ); // uses full URI for justin + } Note that you are using only the Local Part rather than the full string with namespace included. I thought we trying to move away from throwing namespaces in the bin... However, this reminds me of another issue I haven't been able to address yet. Attribute Expressions of the form "full_namespace:property_name" currently cannot be evaluated by the feature property accessors because X-path doesn't accept full namespaces. Perhaps to solve this issue we can use the NamespaceSupport object and the concept of the default namespace? Something like this: public AttributeExpressionImpl( Name name ){         attPath = name.getLocalPart();         schema = null;         this.namespaceSupport = new NamespaceSupport();         namespaceSupport.addPrefix("", name.getNamespaceURI());         this.expressionType = ATTRIBUTE;   }
        Hide
        Niels Charlier added a comment - - edited
        Found this little error in your patch. You wrote this:

        + if( namespaceSupport != null && hints != null ){
        + hints = new Hints(PropertyAccessorFactory.NAMESPACE_CONTEXT, namespaceSupport);
        + }

        While I'm guessing it has to be

        + if( namespaceSupport != null && hints == null ){
        + hints = new Hints(PropertyAccessorFactory.NAMESPACE_CONTEXT, namespaceSupport);
        + }

        Will upload new patch soon
        Show
        Niels Charlier added a comment - - edited Found this little error in your patch. You wrote this: + if( namespaceSupport != null && hints != null ){ + hints = new Hints(PropertyAccessorFactory.NAMESPACE_CONTEXT, namespaceSupport); + } While I'm guessing it has to be + if( namespaceSupport != null && hints == null ){ + hints = new Hints(PropertyAccessorFactory.NAMESPACE_CONTEXT, namespaceSupport); + } Will upload new patch soon
        Hide
        Niels Charlier added a comment -
        new patch
        Show
        Niels Charlier added a comment - new patch
        Hide
        Niels Charlier added a comment -
        Committed, revision 36470

        The commit included, besides what is in the patch, a reworked version of DefaultQueryTest called QueryTest, which tests the new functionality of Query.
        Show
        Niels Charlier added a comment - Committed, revision 36470 The commit included, besides what is in the patch, a reworked version of DefaultQueryTest called QueryTest, which tests the new functionality of Query.
        Hide
        Andrea Aime added a comment -
        Mass closing all issues in resolved state that have not been reopened nor commented over in the last month
        Show
        Andrea Aime added a comment - Mass closing all issues in resolved state that have not been reopened nor commented over in the last month

          People

          • Assignee:
            Niels Charlier
            Reporter:
            Niels Charlier
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: