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

Support `@UnitOfWork` in sub-resources #1959

Merged
merged 3 commits into from Mar 14, 2017

Conversation

Projects
None yet
5 participants
@arteam
Member

arteam commented Mar 13, 2017

Currently Dropwizard doesn't open transactions in sub-resources as reported in #1806. The problem is that Dropwizard scans resource methods for @UnitOfWork during resource initialization and sub-resource methods are not resolved at that time and can't be registered by Dropwizard. The fix is
to defer the lookup of the @UnitOfWork annotation on a method until it's invoked and than cache it. One downside of this approache is that ConcurrentMap doesn't support null values, so we have to use Optional from UnitOfWork which can add an additional overhead, but it should be negligible.

@arteam arteam force-pushed the fix_sublocator_bug branch 3 times, most recently from 63be4e9 to d35af85 Mar 13, 2017

Support `@unitofwork` in sub-resources
Currently, Dropwizard doesn't open transactions in sub-resources as reported
in #1806. The problem is that Dropwizard scans resource methods for
`@unitofwork` during resource initilization and sub-resource methods are
not resolved at that time and can't be registered by Dropwizard. The fix is
to defer the lookup of the `@unitofwork` annotation on a method until it's
invoked and than cache it. One downside of this approache is that
`ConcurrentMap` doesn't support null values, so we have to use `Optional`
from `UnitOfWork` which can add an additional overhead, but it should be
negligible.

@arteam arteam force-pushed the fix_sublocator_bug branch from d35af85 to 3c14c28 Mar 13, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.07%) to 84.754% when pulling 3c14c28 on fix_sublocator_bug into cc84e68 on release/1.1.x.

@jplock jplock added this to the 1.1.0 milestone Mar 13, 2017

@jplock jplock added the improvement label Mar 13, 2017

@jplock

jplock approved these changes Mar 13, 2017

@jeppe-style

This comment has been minimized.

jeppe-style commented Mar 13, 2017

I have an alternative fix for this issue that adds the sub-resource locator methods in the UnitOfWorkApplicationListener (it was done as an university assignment). Please see the code in my fork.

I also created a test for it based on the JerseyIntegrationTest. It has one caveat that when an SQL exception is thrown a MessageBodyProviderNotFoundException is thrown. It is fixed by adding the @Produces(MediaType.APPLICATION_JSON) to the methods annotated with @UnitOfWork.

Running the tests provided by @arteam with this fix the issue is not there though.

jeppe-style added a commit to jeppe-style/dropwizard that referenced this pull request Mar 13, 2017

@arteam

This comment has been minimized.

Member

arteam commented Mar 13, 2017

Thanks! I will take a look at your fix tomorrow.

@arteam

This comment has been minimized.

Member

arteam commented Mar 14, 2017

I had a look and your proposed fix and it works too. Nevertheless, I believe it's better to go with the pull request proposed by me. The motivation is too try to switch from the static lookup to dynamic to avoid guessing completely and delegate it to Jersey. We can use your fix as a backup plan in case the original fix causes any problems.

@joschi

joschi approved these changes Mar 14, 2017

LGTM. 👍

@joschi joschi self-assigned this Mar 14, 2017

joschi added some commits Mar 14, 2017

Update release notes
[ci skip]

@joschi joschi merged commit 5c82b58 into release/1.1.x Mar 14, 2017

@joschi joschi deleted the fix_sublocator_bug branch Mar 14, 2017

joschi added a commit that referenced this pull request Mar 14, 2017

Support `@unitofwork` in sub-resources (#1959)
Closes #1806
(cherry picked from commit 5c82b58)

aaanders added a commit to aaanders/dropwizard that referenced this pull request Apr 26, 2017

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