OpenJPA
  1. OpenJPA
  2. OPENJPA-2197

MethodComparator in AnnotationPersistenceMetaDataParser should also compare parameters

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.0
    • Fix Version/s: 2.3.0
    • Component/s: kernel
    • Labels:
      None

      Description

      AnnotationPersistenceMetaDataParser contains a MethodComparator which only compares the class + the method name. Too bad I have (had...) 2 methods with the same name in my EntityListener:

      @PreUpdate
      public void updateChangeLog(Object entity) { ..

      and also

      private void updateChangeLog(BaseEntity he, ChangeLogEntry cle)

      which is a private helper method.

      Due to the bug in MethodComparator, my @PreUpdate sometimes didn't get detected.

        Activity

        Hide
        Mark Struberg added a comment -

        Yes I feared that. I just saw some old stuff I know form kodo, so most probably either JDO or homegrown stuff

        Show
        Mark Struberg added a comment - Yes I feared that. I just saw some old stuff I know form kodo, so most probably either JDO or homegrown stuff
        Hide
        Albert Lee added a comment -

        Ignore my suggestion! The logic also handles more than the JPA spec required callbacks, which caused some test failures.

        Show
        Albert Lee added a comment - Ignore my suggestion! The logic also handles more than the JPA spec required callbacks, which caused some test failures.
        Hide
        Mark Struberg added a comment -

        We should check this in any case. I assumed that this will be checked already. Can you quickly change the test and verify if it fails with a message if you try to apply a lifecycle callback on another method?

        I went for fixing the Comparator as I didn't like to change the whole logic of this part. Of course, if there are other open tasks in this area, then we might think about doing a clean rework...

        Show
        Mark Struberg added a comment - We should check this in any case. I assumed that this will be checked already. Can you quickly change the test and verify if it fails with a message if you try to apply a lifecycle callback on another method? I went for fixing the Comparator as I didn't like to change the whole logic of this part. Of course, if there are other open tasks in this area, then we might think about doing a clean rework...
        Hide
        Albert Lee added a comment -

        Per spec section "3.5.1 Lifecycle Callback Methods"
        Callback methods defined on an entity class or mapped superclass have the following signature:
        void <METHOD>()
        Callback methods defined on an entity listener class have the following signature:
        void <METHOD>(Object)

        I am wondering if a simpler solution is to detect this requirement rather than check for method signature.

        I.e.
        public static Collection<LifecycleCallbacks>[] parseCallbackMethods
        (Class<?> cls, Collection<LifecycleCallbacks>[] callbacks, boolean sups,
        boolean listener, MetaDataRepository repos) {
        .........
        if (Modifier.isStatic(mods) || Modifier.isFinal(mods) ||
        Object.class.equals(m.getDeclaringClass()))
        continue;

        // ******* addition logic to filter out invalid listeners
        if( m.getParameterTypes().length != (listener?0:1) || m.getReturnType() != Void.class )
        continue;
        // *******

        key = new MethodKey(m);
        if (!seen.contains(key))

        { methods.add(m); seen.add(key); }

        }

        Show
        Albert Lee added a comment - Per spec section "3.5.1 Lifecycle Callback Methods" Callback methods defined on an entity class or mapped superclass have the following signature: void <METHOD>() Callback methods defined on an entity listener class have the following signature: void <METHOD>(Object) I am wondering if a simpler solution is to detect this requirement rather than check for method signature. I.e. public static Collection<LifecycleCallbacks>[] parseCallbackMethods (Class<?> cls, Collection<LifecycleCallbacks>[] callbacks, boolean sups, boolean listener, MetaDataRepository repos) { ......... if (Modifier.isStatic(mods) || Modifier.isFinal(mods) || Object.class.equals(m.getDeclaringClass())) continue; // ******* addition logic to filter out invalid listeners if( m.getParameterTypes().length != (listener?0:1) || m.getReturnType() != Void.class ) continue; // ******* key = new MethodKey(m); if (!seen.contains(key)) { methods.add(m); seen.add(key); } }
        Mark Struberg made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Mark Struberg added a comment -

        fixed in r1339509.

        Show
        Mark Struberg added a comment - fixed in r1339509.
        Hide
        Mark Struberg added a comment -

        We currently use the Method#hashCode in the MethodComparator. But this is really ugly, as it's defined as :
        /**

        • Returns a hashcode for this <code>Method</code>. The hashcode is computed
        • as the exclusive-or of the hashcodes for the underlying
        • method's declaring class name and the method's name.
          */

        Please note that Method#hashCode does NOT cover any method parameters!

        As we do already equals on class and the method name, this doesn't bring any benefit.

        Show
        Mark Struberg added a comment - We currently use the Method#hashCode in the MethodComparator. But this is really ugly, as it's defined as : /** Returns a hashcode for this <code>Method</code>. The hashcode is computed as the exclusive-or of the hashcodes for the underlying method's declaring class name and the method's name. */ Please note that Method#hashCode does NOT cover any method parameters! As we do already equals on class and the method name, this doesn't bring any benefit.
        Mark Struberg made changes -
        Field Original Value New Value
        Attachment OPENJPA-2197_test.patch [ 12527800 ]
        Hide
        Mark Struberg added a comment -

        this test shows the problem. I'm working on a fix right now.

        Show
        Mark Struberg added a comment - this test shows the problem. I'm working on a fix right now.
        Mark Struberg created issue -

          People

          • Assignee:
            Mark Struberg
            Reporter:
            Mark Struberg
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development