[adrian.brock]

After thinking about it for a bit, I think the best thing to do is to go
with a simple implementation of a new DeclaredMethodSignature
since that has a minimal impact.

This won't be integrated with the MethodSignature stuff.

If it turns out in future that integration is required, we can modify the MethodSignature
to support it and rework the DeclaredMethodSignature to be based on it.

That way that calling code doesn't wouldn't need to change.

Although we'd probably deprecate DeclaredMethodSignature if that was the case,
(i.e. if MethodSignature gained an optional declaring class parameter) and remove
it in JBossMDR 3.0?

https://jira.jboss.org/jira/browse/JBMDR-35

[adrian.brock]

If you change your build to use the latest jboss mdr snapshot, you can try this out.

You have a choice of
new DeclaredMethodSignature(Method)
or
new DeclaredMethodSignature(declaringClassName, methodName, parameters);

I'll leave JBMDR-35 open for now, since such hacks always lead to other hacks. :-)

In principle (there's no reason why it shouldn't work) you can
programmatically add INSTANCE metadata
against this signature (e.g. xml overrides), but I haven't tried/tested it.

[wolfc]

The trouble starts in:
AbstractMutableComponentMetaDataLoader.addAnnotation(Member member, T annotation)

The sub component signature becomes MethodSignature instead of DeclaredMethodSignature.

For the moment I'll try to work around this with addAnnotation(Signature signature, T annotation).

[adrian.brock]

"wolfc" wrote:
The trouble starts in:
AbstractMutableComponentMetaDataLoader.addAnnotation(Member member, T annotation)

The sub component signature becomes MethodSignature instead of DeclaredMethodSignature.

For the moment I'll try to work around this with addAnnotation(Signature signature, T annotation).

How's it supposed to know whether addAnnotation(Method, Annotation)
should be a MethodSignature or DeclaredMethodSignature?

A general metadata loader (in this case the memory loader for storing the xml)
doesn't even know what the class is so it can't even search to see whether
it is a private Method that is masked by a different method in that class or one
of its super classes.

[wolfc]

How's it supposed to know whether addAnnotation(Method, Annotation)
should be a MethodSignature or DeclaredMethodSignature?

That's always a declared method, because else you won't be able to follow the Java rules for annotations on methods.

A general metadata loader (in this case the memory loader for storing the xml)
doesn't even know what the class is so it can't even search to see whether
it is a private Method that is masked by a different method in that class or one
of its super classes.

<session>
 <ejb-name>SameMethodNameBean</ejb-name>
 <post-construct>
 <lifecycle-callback-class>SameMethodNameSuper</lifecycle-callback-class>
 <lifecycle-callback-method>postConstructInSuper</lifecycle-callback-method>
 </post-construct>
 </session>

I can see that this should translate to a DeclaredMethodSignature, while

<session>
 <ejb-name>SameMethodNameBean</ejb-name>
 <post-construct>
 <lifecycle-callback-method>postConstructInSuper</lifecycle-callback-method>
 </post-construct>
 </session>

this could be a MethodSignature.

It's just that somewhere the overloading rules for methods must be obeyed, so that in the second xml SameMethodNameSuper.postConstructInSuper doesn't get the PostConstruct annotation.

Right now MethodSignature semantics is horribly broken as AnnotatedElementLoaderNotPublicUnitTestCase.testSameName shows.

[adrian.brock]

"wolfc" wrote:

How's it supposed to know whether addAnnotation(Method, Annotation)
should be a MethodSignature or DeclaredMethodSignature?

That's always a declared method, because else you won't be able to follow the Java rules for annotations on methods.

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". :-)

Right now MethodSignature semantics is horribly broken as AnnotatedElementLoaderNotPublicUnitTestCase.testSameName shows.

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.

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));

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.

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. :-)

[adrian.brock]

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.

If you're willing to put the analysis work into making sure that adding the
declaring class to the signature's equals() doesn't break existing use cases
then I would be willing to change my position.

e.g. the MC "knows" the declaring class when it populates the xml
since it matches the xml elements to ClassInfo/MethodInfo, etc.

 private void updateAnnotations(MutableMetaDataRepository repository, ClassLoader classloader, ComponentMutableMetaData component, KernelControllerContext context, MethodInfo methodInfo, Set<AnnotationMetaData> annotations, boolean add)
 {
 if (annotations == null || annotations.isEmpty())
 return;

 Signature signature = new MethodSignature(methodInfo);
 ScopeKey scope = new ScopeKey(CommonLevels.JOINPOINT_OVERRIDE, methodInfo.getName());
 updateAnnotations(repository, classloader, component, context, signature, scope, annotations, add);
 }

but that doesn't mean everywhere else knows.

If you are not willing to do the analysis, then I'm not prepared to spend
weeks fixing things broken by this change. :-)

[wolfc]

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.

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.

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.

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.

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.

[adrian.brock]

"wolfc" wrote:

I'm happy to use DeclaredMethodSignature though, so if that's the solution please leave that one in. :-)

The idea of DeclaredMethodSignature is to get something working.

I'd much rather use MethodSignature, it's just a case of finding a way to
do it without adding performance penalities to (or even worse breaking)
existing code.

Like I said in an earlier post, once we know how to do that, we can retrofit
DeclaredMethodSignature to be:

@Deprecated // legacy hack
public class DeclaredMethodSignature extends MethodSignature { ... }

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.

Add it if you want, or raise a JIRA issue.

[adrian.brock]

"wolfc" wrote:

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.

Yes, but the point is how do you know which class it is?

Unless you know the exact class/method upfront (the search is pointless then anyway)
the assumption is that it is a method on
AnnotatedElementMetaDataLoader.annotated which is the subclass in your case.

[wolfc]

The idea of DeclaredMethodSignature is to get something working.

I'd much rather use MethodSignature, it's just a case of finding a way to
do it without adding performance penalities to (or even worse breaking)
existing code.

Like I said in an earlier post, once we know how to do that, we can retrofit
DeclaredMethodSignature to be:

@Deprecated // legacy hack
public class DeclaredMethodSignature extends MethodSignature { ... }

Although I agree, I don't see how that's possible. MethodSignature only works if you know the class hierarchy.

class A { void postConstruct() {} }
class B extends A { void postConstruct() {} }

For object A1: MethodSignature postConstruct equals DeclaredMethodSignature A.postConstruct.
For object B1: MethodSignature postConstruct equals DeclaredMethodSignature B.postConstruct.

If I had @PostConstruct on A.postConstruct:
For object A1: has annotation @PostConstruct on MethodSignature postConstruct
For object B1: has no annotation @PostConstruct on MethodSignature postConstruct
while both MethodSignatures are equal.

I would say:

m1 = memoryMetaDataLoaderA.getComponentSignature(methodSignaturePostConstruct);
m2 = memoryMetaDataLoaderA.getComponentSignature(declaredMethodSignatureAPostConstruct);
assert m1.equals(m2);

and

m1 = memoryMetaDataLoaderB.getComponentSignature(methodSignaturePostConstruct);
m2 = memoryMetaDataLoaderB.getComponentSignature(declaredMethodSignatureAPostConstruct);
m3 = memoryMetaDataLoaderB.getComponentSignature(declaredMethodSignatureBPostConstruct);
assert !m1.equals(m2);
assert m1.equals(m3);

Right?

Add it if you want, or raise a JIRA issue.

The assertion only highlights the problem, it doesn't solve anything.