Skip to content
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

Spring Jdbi repositories #2528

Merged
merged 3 commits into from Nov 14, 2023
Merged

Conversation

xfredk
Copy link
Contributor

@xfredk xfredk commented Nov 6, 2023

I wrote some code that scans for sql-object interfaces annotated with @JdbiRepository. These interfaces can be automatically detected by spring and used for autowiring in spring applications. These beans work seamlessly with spring transactions.

Points of notice:

  • I changed the JdbiUtil class in a non-backward compatible way! This was because (I think) the original had a bug.
  • I added two dependencies to the project:
    • a dependency to jdbi3-sqlobject (The main point of this change)
    • a dependency to spring-context (To enable the detection and processing of the annotations)

I considered making a separate project but thought it was close enough to put it in the spring5 project

- add @EnableJdbiRepositories and @JdbiRepository annotations
- add jdbi repository scanner and jdbi repository bean factory
- proxy created extensions for spring transaction support
- (bug fix) close handler when spring transaction is closed
@xfredk xfredk changed the title Spring Jdbi repositories: Spring Jdbi repositories Nov 6, 2023
@hgschmie
Copy link
Contributor

hgschmie commented Nov 7, 2023

Hi @xfredk!

Thank you for your PR to the Jdbi project! I triggered the CI scripts and I will review in a bit.

- fixed codestyle issues
- fixed codestyle issues
Copy link

sonarcloud bot commented Nov 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@xfredk
Copy link
Contributor Author

xfredk commented Nov 9, 2023

De code analysis detected duplication of classes, but I do not know how to solve this problem. (Apparantely spring aop, a transitive dependency of spring context, copied some aopalliance classes into their jar)

Copy link
Member

@stevenschlansker stevenschlansker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this nice improvement. I am not super familiar with the "Spring way" but the code is nicely written and hopefully others will find this useful too.

Class clazz = ClassUtils.resolveClassName(className, null);

Map<String, Object> attributes = getAnnotationAttributes(JdbiRepository.class, annotationMetadata);
String jdbiQualifier = (String) attributes.get("jdbiQualifier");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too bad you cannot use the annotation type itself here, like anno.jdbiQualifier(), but I guess this is how spring does this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check whether I can get it working (easily) by accessing the annotation directly. If so, I will create a new PR for it

return Proxy.getInvocationHandler(target)
.invoke(target, method, args);
} else {
return MethodHandles.lookup().unreflect(method)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is expensive to lookup the method handle every time you invoke the method, but it's probably ok to fix that later when it shows up as an actual performance bottleneck.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur, it is possible to have an initialized object to do this, but the code will end up to be more complex while this code is already complex.

@stevenschlansker
Copy link
Member

Yes, apparently spring-aop copied the aopalliance classes into their jar. That's annoying.

stevenschlansker added a commit that referenced this pull request Nov 14, 2023
this conflicts with spring-aop, which is coming in from #2528
we don't need these annotations in the docs either way.
@stevenschlansker
Copy link
Member

@xfredk , it would be nice to add a little bit about this new feature into the docs if you have time.

stevenschlansker added a commit that referenced this pull request Nov 14, 2023
@stevenschlansker stevenschlansker merged commit 7420d45 into jdbi:master Nov 14, 2023
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants