JBehave
  1. JBehave
  2. JBEHAVE-327

Remove dependency on javassist by using standard JDK annotation functionality

    Details

    • Type: Improvement Improvement
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: Core
    • Labels:
      None
    • Patch Submitted:
      Yes
    • Number of attachments :
      4

      Description

      Use of javassist is typically done for access to non-RUNTIME Retention annotations or for annotation parsing over a large set of classes when you don't want to load the class. We don't have those use cases and thus can eliminate the javassist dependency in AnnotationFinder.

      New version of AnnotationFinder is attached that seems to work - I wasn't sure if getAnnotation should look at interfaces and/or superclasses (not needed if we appropriate add the @Inherited to our annotations).

      jbehave-core/pom.xml also changed to remove the dependency.

      1. 0001-removal-of-javassist.patch
        13 kB
        Brian Repko
      2. 0002-merge-change-of-Exception-type.patch
        1.0 kB
        Brian Repko
      3. 0003-add-support-for-inherited-annotation-values.patch
        7 kB
        Brian Repko
      4. AnnotationFinder.java
        2 kB
        Brian Repko

        Activity

        Hide
        Cristiano Gavião added a comment -

        Brian, JDK don't allow you to merge arrays attributes if you need extend your Embedders and reuse your annotated steps classes...

        It was the mainly reason that I've choosed to use javassist.

        cheers

        Show
        Cristiano Gavião added a comment - Brian, JDK don't allow you to merge arrays attributes if you need extend your Embedders and reuse your annotated steps classes... It was the mainly reason that I've choosed to use javassist. cheers
        Hide
        Brian Repko added a comment -

        Cristiano,

        Thanks for the comment - good to know the "why".

        One can walk the class hierarchy tree and do that work (just like walking interfaces and superclasses to look for un-inherited annotations). That is no different than how Spring-Test's @ContextConfiguration works. But the actual reading of the annotation information doesn't require javassist.

        Do we always combine arrays for every annotation or do we need a boolean parameter on some of those methods? What about interfaces?

        Brian

        Show
        Brian Repko added a comment - Cristiano, Thanks for the comment - good to know the "why". One can walk the class hierarchy tree and do that work (just like walking interfaces and superclasses to look for un-inherited annotations). That is no different than how Spring-Test's @ContextConfiguration works. But the actual reading of the annotation information doesn't require javassist. Do we always combine arrays for every annotation or do we need a boolean parameter on some of those methods? What about interfaces? Brian
        Hide
        Brian Repko added a comment -

        Looked over the annotations and the original AnnotationFinder.

        It looks like we are only talking about Type annotations here, since AnnotationFinder only looks at the class definitions for visible and invisible annotations. Also a scalar (single-valued) property is always overriden by the current logic (and there is no such thing as a missing property on an annotation - it might have a default value - but it has to be there).

        So we only need to design for multi-valued properties in Type annotations. That list includes the following:

        @UsingSteps - instances
        @UsingGuice - modules
        @UsingPico - modules
        @UsingSpring - resources

        All of these have use cases where one would both want to and not want to inherit the values from the super class(es). Fortunately, we only have 1 multi-valued property in each one.

        Spring deals with this situation by adding another property to @ContextConfiguration by having a String[] locations property (what we call resources, with a default of an empty array) and then a boolean inheritLocations property with a default value of true. The code that process this routine walks up the class hierarchy and adds locations to the ultimate array. Spring does this in the same way that AnnotationFinder does. Both use List instead of Set - since the use of the List takes into account duplicates (though, technically, Set would be better).

        I'm not sure if Pico or Guice have annotations that create containers so I'm not sure what those do.

        I would recommend the same pattern.

        @UsingSteps - instances and inheritInstances
        @UsingGuice - modules and inheritModules
        @UsingPico - modules and inheritModules
        @UsingSpring - resources and inheritResources

        The problem is now that there is no known property - we would have to code for realizing that the value is an array and then build the name of the inheriting property and check that up the class hierarchy.

        But I also think that the existing code, which always inherits/appends isn't the best option either. There are use cases where one wants to turn that off.

        What do people think? I'd prefer to have some buy-in before coding.

        Show
        Brian Repko added a comment - Looked over the annotations and the original AnnotationFinder. It looks like we are only talking about Type annotations here, since AnnotationFinder only looks at the class definitions for visible and invisible annotations. Also a scalar (single-valued) property is always overriden by the current logic (and there is no such thing as a missing property on an annotation - it might have a default value - but it has to be there). So we only need to design for multi-valued properties in Type annotations. That list includes the following: @UsingSteps - instances @UsingGuice - modules @UsingPico - modules @UsingSpring - resources All of these have use cases where one would both want to and not want to inherit the values from the super class(es). Fortunately, we only have 1 multi-valued property in each one. Spring deals with this situation by adding another property to @ContextConfiguration by having a String[] locations property (what we call resources, with a default of an empty array) and then a boolean inheritLocations property with a default value of true. The code that process this routine walks up the class hierarchy and adds locations to the ultimate array. Spring does this in the same way that AnnotationFinder does. Both use List instead of Set - since the use of the List takes into account duplicates (though, technically, Set would be better). I'm not sure if Pico or Guice have annotations that create containers so I'm not sure what those do. I would recommend the same pattern. @UsingSteps - instances and inheritInstances @UsingGuice - modules and inheritModules @UsingPico - modules and inheritModules @UsingSpring - resources and inheritResources The problem is now that there is no known property - we would have to code for realizing that the value is an array and then build the name of the inheriting property and check that up the class hierarchy. But I also think that the existing code, which always inherits/appends isn't the best option either. There are use cases where one wants to turn that off. What do people think? I'd prefer to have some buy-in before coding.
        Hide
        Brian Repko added a comment -

        Also, do we want to extend the use of @Using* annotations to interfaces?

        Show
        Brian Repko added a comment - Also, do we want to extend the use of @Using* annotations to interfaces?
        Hide
        Brian Repko added a comment -

        Attaching patch files (finally learned git format-patch) for all changes including the latest which is the implementation and test of the above change.

        Show
        Brian Repko added a comment - Attaching patch files (finally learned git format-patch) for all changes including the latest which is the implementation and test of the above change.
        Hide
        Brian Repko added a comment -

        Not sure how great of a "patch" these are but you should be able to apply these...

        Show
        Brian Repko added a comment - Not sure how great of a "patch" these are but you should be able to apply these...
        Hide
        Mauro Talevi added a comment -

        Actually, I'm having trouble applying them. My patch bin does not understand them.

        Could you simply try recreating them from command line using git diff:

        git diff > <name>.patch

        Thanks

        Show
        Mauro Talevi added a comment - Actually, I'm having trouble applying them. My patch bin does not understand them. Could you simply try recreating them from command line using git diff: git diff > <name>.patch Thanks
        Hide
        Mauro Talevi added a comment - - edited

        Applied patches 1 and 2 (after battling with issues with trailing whitespace).

        Not yet applied 3: I agree with making the inheritance behaviour configurable, but would like to avoid proliferation of boolean properties in multiple annotations. Rather, we could introduce another separate annotation, say @UsingInheritance, that could be used independently of the method of composition used. WDYT?

        Show
        Mauro Talevi added a comment - - edited Applied patches 1 and 2 (after battling with issues with trailing whitespace). Not yet applied 3: I agree with making the inheritance behaviour configurable, but would like to avoid proliferation of boolean properties in multiple annotations. Rather, we could introduce another separate annotation, say @UsingInheritance, that could be used independently of the method of composition used. WDYT?
        Hide
        Mauro Talevi added a comment -

        Resolving this issue, as the dependency on javassist as been removed.

        Inheritance issue now tracked by JBEHAVE-329.

        Show
        Mauro Talevi added a comment - Resolving this issue, as the dependency on javassist as been removed. Inheritance issue now tracked by JBEHAVE-329 .

          People

          • Assignee:
            Mauro Talevi
            Reporter:
            Brian Repko
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: