-
Notifications
You must be signed in to change notification settings - Fork 274
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
Moved transaction handling into Handle instead of @Transactional #4966
Conversation
Qodana Community for JVM38 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I liked the automatic thread-local nesting, but nothing wrong with being explicit.
@@ -687,7 +679,6 @@ private void insertReferences(Handle handle, Long contentId, List<ArtifactRefere | |||
} | |||
|
|||
@Override | |||
@Transactional | |||
public List<String> deleteArtifact(String groupId, String artifactId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the GA/GAV value types everywhere to reduce accidental errors, since String is a String is a String. I'd like to add value types for other identifiers in the future, such as hashes and numeric IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out-of-scope for this PR though.
app/src/main/java/io/apicurio/registry/storage/impl/sql/SQLServerSqlStatements.java
Outdated
Show resolved
Hide resolved
if (canonical) { | ||
var artifactMetaData = getArtifactMetaData(groupId, artifactId); | ||
var artifactMetaData = getArtifactMetaDataRaw(handle, groupId, artifactId); | ||
Function<List<ArtifactReferenceDto>, Map<String, TypedContent>> referenceResolver = (refs) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate lambda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean because this lamba is the same as another one in this same class, it's because they need to be local in order to pass the local Handle
reference.
Original work by @jsenko here: