New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NPE of non-resource sub-resource methods #1718

Merged
merged 1 commit into from Sep 2, 2016

Conversation

Projects
None yet
4 participants
@nickbabcock
Contributor

nickbabcock commented Sep 2, 2016

Closes #1716
Supersedes #1717

Whether it is a good idea to have a method return Object can be left up to discussion, but the code should not throw a NPE.

@evnm

This comment has been minimized.

Member

evnm commented Sep 2, 2016

Ah, I see. The issue occurs when the resource method lacks an HTTP verb annotation. When I change the interface-returning resource method in #1717 to instead return Object, the tests still pass because I'd added @GET.

Thanks for taking care of this, Nick. LGTM

@evnm evnm added this to the 1.0.1 milestone Sep 2, 2016

@evnm evnm added the improvement label Sep 2, 2016

@coveralls

This comment has been minimized.

coveralls commented Sep 2, 2016

Coverage Status

Coverage increased (+0.008%) to 82.306% when pulling b8134bc on nickbabcock:non-resource-endpoint-log into 68b1af9 on dropwizard:master.

@apatrida

This comment has been minimized.

apatrida commented Sep 2, 2016

@nickbabcock

Whether it is a good idea to have a method return Object can be left up to discussion, but the code should not throw a NPE.

This judgement was already made by Jersey as promoted + acceptable. As you see in https://jersey.java.net/documentation/latest/jaxrs-resources.html#d0e2496 there is a section talking about polymorphism that shows using Object (or in our case Any) as a valid model. So it is recommended by Jersey and yet crashed Dropwizard. (all about sub-resources here: https://jersey.java.net/documentation/latest/jaxrs-resources.html#d0e2592)

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Sep 2, 2016

Ah sorry, my statement was misleading. Everything that Jersey allows, Dropwizard should allow. I meant to convey that the return type should be as specific as possible 😝

nickbabcock added a commit to nickbabcock/dropwizard that referenced this pull request Sep 2, 2016

nickbabcock added a commit to nickbabcock/dropwizard that referenced this pull request Sep 2, 2016

nickbabcock added a commit to nickbabcock/dropwizard that referenced this pull request Sep 2, 2016

arteam added a commit that referenced this pull request Sep 21, 2016

arteam added a commit that referenced this pull request Sep 21, 2016

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