[jboss-dev-forums] [Design the new POJO MicroContainer] - Re: MDR doesn't work on annotated privates

[wolfc]

"adrian at jboss.org" wrote : Not it's not a DeclaredMethodSignature. The metadata contexts associated
  | with those are not populated with the data loaded into MethodSignatures.
  | 
  | The DeclaredMethodSignatures is a hack so you can shadow
  | methods on the super class.
  | 
  | It's not the same semantic. The semantic is designed to be the same as
  | Class.get{Declared}Method(String name, Class.. parameters);
  | the idea is you don't need to know the method/class, only its signature,
  | that's why the class/key is called "Signature". :-)
If get{Declared}Method was a static method I would agree with you. It's not, there always is a class in play.
"adrian at jboss.org" wrote : No it's not. It just does not do the horrible semantic you want it to do
  | which is why we introduced the DeclaredMethodSignature.
  | 
  | The test currently passes anyway, but then it is a useless test.
Hehe, actually it does my semantic. That's the horrible thing about it. :-)
This one does your semantic:
   public void testMutable() throws Exception
  |    {
  |       MemoryMetaDataLoader loader = new MemoryMetaDataLoader();
  |       MetaData metaData = new MetaDataRetrievalToMetaDataBridge(loader);
  |       
  |       Method superMethod = SuperClassOfNotPublic.class.getDeclaredMethod("sameName");
  |       loader.addAnnotation(superMethod, new TestAnnotationImpl());
  |       MetaData superMethodMetaData = metaData.getComponentMetaData(Signature.getSignature(superMethod));
  |       assertAnnotation(superMethodMetaData, TestAnnotation.class);
  |       
  |       Method method = NotPublic.class.getDeclaredMethod("sameName");
  |       MetaData methodMetaData = metaData.getComponentMetaData(Signature.getSignature(method));
  |       assertNoAnnotation(methodMetaData, TestAnnotation.class);
  |    }
And it fails in the last assert (because I wrote it according to my semantics). It's exactly the same as the AnnotatedMetaDataLoader test.
"adrian at jboss.org" wrote : The test that demonstrates the semantic you asked for is
  | AnnotatedElementLoaderDeclaredMethodSignatureTestCase.
  | 
  | The root of your difficulty is this call
  | 
  |   |       MetaData superMethodMetaData = metaData.getComponentMetaData(Signature.getSignature(superMethod));
  |   | 
  | 
  | which doesn't return the superMethod metadata.
  | 
  | When you should be doing
  | 
  | 
  |   | MetaData superMethodMetaData = metaData.getComponentMetaData(new DeclaredMethodSignature(superMethod));
  |   | 
This is were I'm going now. I now pass in the DeclaredMethodSignature and it works out fine.
"adrian at jboss.org" wrote : We could add a new method:
  | 
  |   |    public static Signature getSignature(Class<?> clazz, Member member)
  |   |    {
  |   |       if (member == null)
  |   |          throw new IllegalArgumentException("Null member");
  |   |       
  |   |       if (member instanceof Method)
  |   |       {
  |   |          Method method = Method.class.cast(member);
  |   |          Method other = method;
  |   |          // See if we are masking the method
  |   |          if (clazz != method.getDeclaringClass())
  |   |             other = ReflectionUtils.findMethod(clazz, method);
  |   |          // Yes
  |   |          if (other.equals(method) == false)
  |   |             return new DeclaredMethodSignature(member);
  |   |          // No
  |   |          else
  |   |             return new MethodSignature(method);
  |   |       }
  |   | ...
  |   | 
  | 
  | But then it would be horribly slow for all other users.
I don't want to impose performance penalties for all.

We already have code in EJB3 which checks for overloading.
"adrian at jboss.org" wrote : Alternatively, we could change MethodSIgnature to include the "DeclaringClass"
  | in its equals(). But then people who don't know the DeclaringClass,
  | e.g. populating metadata from xml would have to do the findMethod().getDeclaringClass()
  | upfront to populate the MethodSignature, otherwise the equals() would no longer work.
  | 
  | Since Signature.equals() is designed to work before the classes are loaded
  | that would be difficult - though not impossible with javassist, 
  | 
  | Nothing actually uses it that way at the moment, but there are obvious performance
  | improvements (on the roadmap) to be gained by not duplicating annotation
  | loading, javassist parsing and caching work across AOP, annotation scanning, ClassInfo, etc.
  | 
  | If you continue to just say it is a bug, instead of suggesting how we make
  | your semantic work without breaking (including performance) every other
  | use case, I'd be inclined to just remove the *experimental* DeclaredMethodSignature
  | and tell you to do the hack yourself in your code. :-)
Oi, I've already refactored ejb3-metadata on top of it.

I just find it weird that AnnotatedElementMetaDataLoader semantics have changed with JBMDR-28 (in my favour though :-) ). And that MemoryMetaDataLoader doesn't adhere to Java semantics. (If used in conjunction with MethodSignature.)
It also means that both MetaDataLoaders do not behave in the same fashion.
This nags me.

I'm happy to use DeclaredMethodSignature though, so if that's the solution please leave that one in. :-)
I still think there should be some assertion that prevents me from abusing MethodSignature though.

assert method == ReflectionUtils.findMethod(clazz, signature.getName(), signature.getParametersTypes(clazz)) : "You're abusing JBMDR-28";
In AnnotatedMetaDataLoader? It doesn't impose performance downgrades, but it's not really friendly/usable.

View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4170776#4170776

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4170776