Skip to content
This repository was archived by the owner on May 28, 2018. It is now read-only.

Conversation

@ccronemberger
Copy link

...ierarchy. Without this fix it is not possible to register proxied beans created by Spring because the annotation is in the super class

…r hierarchy. Without this fix it is not possible to register proxied beans created by Spring because the annotation is in the super class
@jerseyrobot
Copy link
Contributor

Can one of the admins verify this patch?

@ccronemberger
Copy link
Author

@mgajdos
Copy link
Contributor

mgajdos commented Jul 4, 2014

Hi Constantino,

thank you for the patch. Can you, please, include a test as well? Also, please, change the copyright year to 2013-2014.

In order to be able to accept your contribution, we need you to sign Oracle Contributor Agreement (OCA). You can find more details here:

https://jersey.java.net/scm.html#/Submitting_Patches_and_Contribute_Code
http://www.oracle.com/technetwork/community/oca-486395.html

Michal

@ccronemberger
Copy link
Author

Hi,
I have just sent the OCA form and pushed the test class.
Regards,
Constantino.

@mgajdos
Copy link
Contributor

mgajdos commented Jul 4, 2014

Jenkins, please test this patch.

@ccronemberger
Copy link
Author

I saw the test failed, but it is giving errors in the last 4 builds, so most likely the problem was introduced at that time. The error is related with some ports not being available: either it is trying to run 2 tests that use the same port in parallel or another build in the same machine is interfering with Jersey build.

@ccronemberger
Copy link
Author

The errors are:

org.glassfish.jersey.test.spi.TestContainerException: java.net.BindException: Address already in use
at org.glassfish.jersey.test.grizzly.GrizzlyTestContainerFactory$GrizzlyTestContainer.start(GrizzlyTestContainerFactory.java:112)
at org.glassfish.jersey.test.JerseyTest.setUp(JerseyTest.java:613)
...
Caused by: java.net.BindException: Address already in use
at sun.nio.ch.Net.bind0(Native Method)
at sun.nio.ch.Net.bind(Net.java:444)
at sun.nio.ch.Net.bind(Net.java:436)
at sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:214)
...

@mpotociar
Copy link
Collaborator

Jenkins, retest this patch.

@mgajdos
Copy link
Contributor

mgajdos commented Jul 14, 2014

Verified manually. We'll merge this pull request once we get confirmation about your OCA.

@ccronemberger
Copy link
Author

Hi,

My OCA was just approved.

Thanks,
Constantino

@AdamLindenthal
Copy link
Member

Jenkins, retest this patch.

AdamLindenthal pushed a commit that referenced this pull request Jul 17, 2014
Make the method really search for the @path annotation in the ancestor h...
@AdamLindenthal AdamLindenthal merged commit dce250e into javaee:master Jul 17, 2014
@AdamLindenthal
Copy link
Member

Thank you for your contribution, I just merged your pull request.

@bagana
Copy link

bagana commented Jul 12, 2015

Hi there,

I found this fix by Googling about a different but probably related problem of jersey-spring3.

After hours of time debugging, I concluded that my @Context-annotated fields in Jersey resource and filter classes are ignored once I apply a Spring @Transactional on them. So I suspected that could it be Spring providing sub-classed instances to Jersey, and jersey didn't search the super classes for @Context annotation. I checked the diff of this fix but @Context seems to be handled somewhere else. I'm not familiar with Spring internal, nor with Jersey's. Can anyone give me some direction?

I tried with Jersey everything 2.19, Spring everything 4.1.6.

Thank you!

@ccronemberger
Copy link
Author

Hi,

That was exactly the use case I had. The workaround I used was to let Spring create the beans and register them on Jersey after the proxy is created. When I first implemented this I had to fix another problem in Jersey, but now the fix is already integrated on the latest release, so it should work the way I described above.

Let me know if you have further questions.

Regards,

    Constantino

Enviado do Yahoo Mail no Android

De:"Bagana" notifications@github.com
Data:11:47 dom, 12 de jul de AM
Assunto:Re: [jersey] Make the method really search for the @path annotation in the ancestor h... (#90)

Hi there,

I found this fix by Googling about a different but probably related problem of jersey-spring3.

After hours of time debugging, I concluded that my @Context-annotated fields in Jersey resource and filter classes are ignored once I apply a Spring @transactional on them. So I suspected that could it be Spring providing sub-classed instances to Jersey, and jersey didn't search the super classes for @context annotation. I checked the diff of this fix but @context seems to be handled somewhere else. I'm not familiar with Spring internal, nor with Jersey's. Can anyone give me some direction?

I tried with Jersey everything 2.19, Spring everything 4.1.6.

Thank you!


Reply to this email directly or view it on GitHub.

@bagana
Copy link

bagana commented Jul 16, 2015

Thanks for the reply and the idea, @ccronemberger.

I came up with this solution:

    // "static", as long as it's singleton, should be fine.
    // @Component makes it singleton anyway.
    private static ResourceInfo resourceInfo;

    // "final", so it won't be overridden by Spring (cglib).
    // prints a warning message, though. (INFO level)
    @ Context
    public final void setResourceInfo( final ResourceInfo info) {
        resourceInfo = info;
    }

    @ Override
    public void filter( final ContainerRequestContext request) throws IOException {
        resourceInfo.getResourceMethod()...
        ...
    }

So far it seems working fine. I guess this would probably be a better workaround, because if the problem should be solved in a future version, I only need to remove two key words.

@mpotociar
Copy link
Collaborator

I'm afraid the solution in this patch is not correct. The JSR-339 specification in chapter 3.6 clearly states that annotations on a superclass take precedence over annotations on interfaces:

JAX-RS annotations may be used on the methods and method parameters of a super-class or an implemented interface. Such annotations are inherited by a corresponding sub-class or implementation class method provided that the method and its parameters do not have any JAX-RS annotations of their own. Annotations on a super-class take precedence over those on an implemented interface. The precedence over conflicting annotations defined in multiple implemented interfaces is implementation specific. Note that inheritance of class or interface annotations is not supported.

@ccronemberger
Copy link
Author

Yes, I understand that inheritance is supported only for methods and method parameters and not for classes. The patch is for classes, so it is clearly violating the spec.
The option in this case would be to generate the subclasses with the annotations, so they don't need to be inherited.

Besides disabling the fix it would be a good idea to add a comment to the code explaining why inheritance is not desirable in this case and ideally also explain why the spec states this. It would be nice to also have the test reworked so it can validate that it will not be implemented again.

@AdamLindenthal
Copy link
Member

Hi, Marek was originally pointing out, that the search order is opposite than the specs requires. The other thing is, that the spec states that "inheritance of class or interface annotations is not supported".

Well, if we should interpret that strictly, we would have to remove even the search in the interfaces already present before this patch. For now, we decided to interpret this statement more freely in the manner "this behaviour is neither specified, nor prohibited by the spec", so we just adjusted the patch so that it searches in the same order as specified for methods (superclasses first, interfaces if not found elsewhere).

Your use case should still work and Jersey behaves at least in the spirit of the spec...

Regards,
Adam

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants