-
Notifications
You must be signed in to change notification settings - Fork 725
SONARJAVA-6539 Create rule S8947: JPA entity classes should not be final #5719
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ebd2db5
implement check
NoemieBenard b5b7a77
fix autoscan
NoemieBenard e82572a
add check on final methods
NoemieBenard 4268b8f
fix fp on static/private methods
NoemieBenard abc7f8e
fix autoscan
NoemieBenard 48c1909
update rule metadata
NoemieBenard d993cea
avoid fp with classes implementing an interface
NoemieBenard File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
6 changes: 6 additions & 0 deletions
6
its/autoscan/src/test/resources/autoscan/diffs/diff_S8947.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "ruleKey": "S8947", | ||
| "hasTruePositives": false, | ||
| "falseNegatives": 7, | ||
| "falsePositives": 0 | ||
| } |
84 changes: 84 additions & 0 deletions
84
java-checks-test-sources/default/src/main/java/checks/JpaEntityFinalCheckJakartaSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| package checks; | ||
|
|
||
| import jakarta.persistence.Entity; | ||
| import jakarta.persistence.Id; | ||
| import jakarta.persistence.MappedSuperclass; | ||
|
|
||
| @Entity | ||
| final class JpaEntityFinalCheckJakartaFinalEntity { // Noncompliant {{Remove this "final" modifier from this JPA entity class.}} | ||
| //^[sc=1;ec=5] | ||
| @Id | ||
| private Long id; | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
| } | ||
|
|
||
| @MappedSuperclass | ||
| final class JpaEntityFinalCheckJakartaFinalMappedSuperclass { // Noncompliant {{Remove this "final" modifier from this JPA entity class.}} | ||
| //^[sc=1;ec=5] | ||
| @Id | ||
| private Long id; | ||
| } | ||
|
|
||
| @Entity | ||
| class JpaEntityFinalCheckJakartaEntityWithFinalMethod { // Compliant - class itself is not final | ||
| @Id | ||
| private Long id; | ||
|
|
||
| public final Long getId() { // Noncompliant {{Remove this "final" modifier from this JPA entity method.}} | ||
| // ^^^^^ | ||
| return id; | ||
| } | ||
|
|
||
| public Long getIdCompliant() { // Compliant | ||
| return id; | ||
| } | ||
|
|
||
| private final Long getIdPrivate() { // Compliant - private methods cannot be overridden by proxies | ||
| return id; | ||
| } | ||
|
|
||
| public static final Long getIdStatic() { // Compliant - static methods cannot be overridden by proxies | ||
| return 0L; | ||
| } | ||
| } | ||
|
|
||
| @MappedSuperclass | ||
| class JpaEntityFinalCheckJakartaMappedSuperclassWithFinalMethod { // Compliant - class itself is not final | ||
| @Id | ||
| private Long id; | ||
|
|
||
| public final Long getId() { // Noncompliant {{Remove this "final" modifier from this JPA entity method.}} | ||
| // ^^^^^ | ||
| return id; | ||
| } | ||
| } | ||
|
|
||
| @Entity | ||
| class JpaEntityFinalCheckJakartaCompliantEntity { // Compliant | ||
| @Id | ||
| private Long id; | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
| } | ||
|
|
||
| @MappedSuperclass | ||
| class JpaEntityFinalCheckJakartaCompliantMappedSuperclass { // Compliant | ||
| @Id | ||
| private Long id; | ||
| } | ||
|
|
||
| class JpaEntityFinalCheckJakartaNotAnEntity { // Compliant - not a JPA entity | ||
| } | ||
|
|
||
| final class JpaEntityFinalCheckJakartaFinalNotAnEntity { // Compliant - not a JPA entity | ||
| } | ||
|
|
||
| class JpaEntityFinalCheckJakartaNotAnEntityWithFinalMethod { // Compliant - not a JPA entity | ||
| public final void doSomething() { // Compliant - not a JPA entity | ||
| } | ||
| } |
52 changes: 52 additions & 0 deletions
52
java-checks-test-sources/default/src/main/java/checks/JpaEntityFinalCheckJavaxSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| package checks; | ||
|
|
||
| import javax.persistence.Entity; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.MappedSuperclass; | ||
|
|
||
| @Entity | ||
| final class JpaEntityFinalCheckJavaxFinalEntity { // Noncompliant {{Remove this "final" modifier from this JPA entity class.}} | ||
| //^[sc=1;ec=5] | ||
| @Id | ||
| private Long id; | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
| } | ||
|
|
||
| @MappedSuperclass | ||
| final class JpaEntityFinalCheckJavaxFinalMappedSuperclass { // Noncompliant {{Remove this "final" modifier from this JPA entity class.}} | ||
| //^[sc=1;ec=5] | ||
| @Id | ||
| private Long id; | ||
| } | ||
|
|
||
| @Entity | ||
| class JpaEntityFinalCheckJavaxEntityWithFinalMethod { // Compliant - class itself is not final | ||
| @Id | ||
| private Long id; | ||
|
|
||
| public final Long getId() { // Noncompliant {{Remove this "final" modifier from this JPA entity method.}} | ||
| // ^^^^^ | ||
| return id; | ||
| } | ||
|
|
||
| private final Long getIdPrivate() { // Compliant - private methods cannot be overridden by proxies | ||
| return id; | ||
| } | ||
|
|
||
| public static final Long getIdStatic() { // Compliant - static methods cannot be overridden by proxies | ||
| return 0L; | ||
| } | ||
| } | ||
|
|
||
| @Entity | ||
| class JpaEntityFinalCheckJavaxCompliantEntity { // Compliant | ||
| @Id | ||
| private Long id; | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
| } |
83 changes: 83 additions & 0 deletions
83
java-checks/src/main/java/org/sonar/java/checks/JpaEntityFinalCheck.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import java.util.List; | ||
| import org.sonar.check.Rule; | ||
| import org.sonar.java.model.ModifiersUtils; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.semantic.Symbol; | ||
| import org.sonar.plugins.java.api.semantic.SymbolMetadata; | ||
| import org.sonar.plugins.java.api.tree.ClassTree; | ||
| import org.sonar.plugins.java.api.tree.MethodTree; | ||
| import org.sonar.plugins.java.api.tree.Modifier; | ||
| import org.sonar.plugins.java.api.tree.ModifierKeywordTree; | ||
| import org.sonar.plugins.java.api.tree.Tree; | ||
|
|
||
| @Rule(key = "S8947") | ||
| public class JpaEntityFinalCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
| private static final List<String> ENTITY_ANNOTATIONS = List.of( | ||
| "javax.persistence.Entity", | ||
| "jakarta.persistence.Entity", | ||
| "javax.persistence.MappedSuperclass", | ||
| "jakarta.persistence.MappedSuperclass" | ||
| ); | ||
|
|
||
| @Override | ||
| public List<Tree.Kind> nodesToVisit() { | ||
| return List.of(Tree.Kind.CLASS, Tree.Kind.METHOD); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitNode(Tree tree) { | ||
| if (tree.is(Tree.Kind.CLASS)) { | ||
| visitClass((ClassTree) tree); | ||
| } else { | ||
| visitMethod((MethodTree) tree); | ||
| } | ||
| } | ||
|
|
||
| private void visitClass(ClassTree classTree) { | ||
| if (!isJpaEntity(classTree.symbol().metadata())) { | ||
| return; | ||
| } | ||
| ModifierKeywordTree finalModifier = ModifiersUtils.getModifier(classTree.modifiers(), Modifier.FINAL); | ||
| if (finalModifier != null) { | ||
| reportIssue(finalModifier, "Remove this \"final\" modifier from this JPA entity class."); | ||
| } | ||
| } | ||
|
|
||
| private void visitMethod(MethodTree methodTree) { | ||
| ModifierKeywordTree finalModifier = ModifiersUtils.getModifier(methodTree.modifiers(), Modifier.FINAL); | ||
| if (finalModifier == null) { | ||
| return; | ||
| } | ||
| Symbol methodSymbol = methodTree.symbol(); | ||
| if (methodSymbol.isStatic() || methodSymbol.isPrivate()) { | ||
| return; | ||
| } | ||
| Symbol.TypeSymbol enclosingClass = methodSymbol.enclosingClass(); | ||
| if (enclosingClass != null && isJpaEntity(enclosingClass.metadata())) { | ||
| reportIssue(finalModifier, "Remove this \"final\" modifier from this JPA entity method."); | ||
| } | ||
| } | ||
|
|
||
| private static boolean isJpaEntity(SymbolMetadata metadata) { | ||
| return ENTITY_ANNOTATIONS.stream().anyMatch(metadata::isAnnotatedWith); | ||
| } | ||
| } | ||
41 changes: 41 additions & 0 deletions
41
java-checks/src/test/java/org/sonar/java/checks/JpaEntityFinalCheckTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.sonar.java.checks.verifier.CheckVerifier; | ||
|
|
||
| import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; | ||
|
|
||
| class JpaEntityFinalCheckTest { | ||
|
|
||
| @Test | ||
| void testWithJakarta() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/JpaEntityFinalCheckJakartaSample.java")) | ||
| .withCheck(new JpaEntityFinalCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void testWithJavax() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/JpaEntityFinalCheckJavaxSample.java")) | ||
| .withCheck(new JpaEntityFinalCheck()) | ||
| .verifyIssues(); | ||
| } | ||
| } |
76 changes: 76 additions & 0 deletions
76
sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8947.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| <p>This issue arises when Java classes annotated with <code>@Entity</code> or <code>@MappedSuperclass</code> are declared <code>final</code>, or when | ||
|
gitar-bot[bot] marked this conversation as resolved.
|
||
| methods within these classes are declared <code>final</code>. JPA providers require the ability to create proxy subclasses for lazy loading and other | ||
| runtime optimizations, which is prevented by the <code>final</code> modifier.</p> | ||
| <h2>Why is this an issue?</h2> | ||
| <p>JPA (Java Persistence API) providers like Hibernate rely on runtime proxy generation to implement several key features:</p> | ||
| <ul> | ||
| <li><strong>Lazy loading</strong>: Loading associated entities only when they are actually accessed, rather than eagerly fetching everything | ||
| upfront</li> | ||
| <li><strong>Dirty checking</strong>: Tracking which fields have changed to optimize database updates</li> | ||
| <li><strong>Performance optimizations</strong>: Creating lightweight proxies that defer expensive operations</li> | ||
| </ul> | ||
| <p>To create these proxies, the JPA provider needs to generate a subclass of your entity class at runtime. This subclass overrides methods to add the | ||
| lazy loading and tracking behavior.</p> | ||
| <p>When you declare a class or method as <code>final</code>, you prevent inheritance and method overriding. This breaks the proxy mechanism:</p> | ||
| <ul> | ||
| <li>A <code>final</code> class cannot be subclassed, so no proxy can be created</li> | ||
| <li>A <code>final</code> method cannot be overridden, so the JPA provider cannot intercept calls to implement lazy loading</li> | ||
| </ul> | ||
| <p>Without working proxies, lazy loading fails. Instead of loading data on demand, the JPA provider may fall back to eager loading, which can cause | ||
| significant performance problems. In some cases, it may even cause runtime exceptions.</p> | ||
| <h3>What is the potential impact?</h3> | ||
| <p>When JPA entities or their methods are marked as <code>final</code>, the application can experience:</p> | ||
| <ul> | ||
| <li><strong>Performance degradation</strong>: Lazy loading fails, forcing the application to eagerly load entire object graphs. This can result in | ||
| loading hundreds or thousands of unnecessary records from the database, significantly slowing down queries.</li> | ||
| <li><strong>Increased memory consumption</strong>: Eagerly loading large object graphs consumes excessive memory, potentially leading to | ||
| OutOfMemoryErrors in production.</li> | ||
| <li><strong>Runtime exceptions</strong>: Some JPA providers throw exceptions when they cannot create required proxies, causing application | ||
| failures.</li> | ||
| <li><strong>Unpredictable behavior</strong>: The application may work correctly in development or testing (with small datasets) but fail or perform | ||
| poorly in production (with realistic data volumes).</li> | ||
| </ul> | ||
| <h2>How to fix it</h2> | ||
| <p>Remove the <code>final</code> modifier from the entity class declaration or its methods. This allows the JPA provider to create proxy subclasses | ||
| for lazy loading and other optimizations. If you are using Hibernate, you can also implement an interface that declares all the attribute | ||
| getters/setters.</p> | ||
| <h3>Code examples</h3> | ||
| <h4>Noncompliant code example</h4> | ||
| <pre data-diff-id="1" data-diff-type="noncompliant"> | ||
| @Entity | ||
| public final class User { // Noncompliant | ||
| @Id | ||
| private Long id; | ||
|
|
||
| private String username; | ||
|
|
||
| @OneToMany(fetch = FetchType.LAZY) | ||
| private List<Order> orders; | ||
|
|
||
| // getters and setters | ||
| } | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <pre data-diff-id="1" data-diff-type="compliant"> | ||
| @Entity | ||
| public class User { | ||
| @Id | ||
| private Long id; | ||
|
|
||
| private String username; | ||
|
|
||
| @OneToMany(fetch = FetchType.LAZY) | ||
| private List<Order> orders; | ||
|
|
||
| // getters and setters | ||
| } | ||
| </pre> | ||
| <h2>Resources</h2> | ||
| <h3>Documentation</h3> | ||
| <ul> | ||
| <li>Hibernate ORM User guide - <a href="https://docs.hibernate.org/stable/orm/userguide/html_single/#entity-pojo-final">Prefer non-final | ||
| classes</a></li> | ||
| <li>Jakarta Persistence Specification - <a href="https://jakarta.ee/specifications/persistence/3.1/jakarta-persistence-spec-3.1.html#a18">The Entity | ||
| Class</a></li> | ||
| </ul> | ||
|
|
||
26 changes: 26 additions & 0 deletions
26
sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8947.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| { | ||
| "title": "JPA entity classes and methods should not be final", | ||
| "type": "BUG", | ||
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ | ||
| "jpa", | ||
| "hibernate", | ||
| "pitfall" | ||
| ], | ||
| "defaultSeverity": "Critical", | ||
| "ruleSpecification": "RSPEC-8947", | ||
| "sqKey": "S8947", | ||
| "scope": "Main", | ||
| "quickfix": "unknown", | ||
| "code": { | ||
| "impacts": { | ||
| "RELIABILITY": "HIGH", | ||
| "MAINTAINABILITY": "MEDIUM" | ||
| }, | ||
| "attribute": "LOGICAL" | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -537,6 +537,7 @@ | |
| "S8909", | ||
| "S8911", | ||
| "S8924", | ||
| "S8947", | ||
| "S8948" | ||
| ] | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.