-
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "ruleKey": "S8947", | ||
| "hasTruePositives": false, | ||
| "falseNegatives": 4, | ||
| "falsePositives": 0 | ||
| } |
| 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 | ||
| } | ||
| } |
| 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; | ||
| } | ||
| } |
| 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); | ||
| } | ||
| } | ||
| 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(); | ||
| } | ||
| } |
| 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. This allows the JPA provider to create proxy subclasses for lazy loading | ||
| and other optimizations.</p> | ||
|
Comment on lines
+33
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Quality: 'How to fix it' section omits final methodsThe rule now also detects Was this helpful? React with 👍 / 👎 |
||
| <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>JPA Specification - <a href="https://jakarta.ee/specifications/persistence/3.0/jakarta-persistence-spec-3.0.html#a121">Official JPA | ||
| specification section on entity class requirements</a></li> | ||
| <li>Hibernate User Guide - <a | ||
| href="https://docs.jboss.org/hibernate/orm/current/userguide/html_single/Hibernate_User_Guide.html#BytecodeEnhancement">Hibernate documentation | ||
| explaining proxy generation and bytecode enhancement</a></li> | ||
| </ul> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| { | ||
| "title": "JPA entity classes 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" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -537,6 +537,7 @@ | |
| "S8909", | ||
| "S8911", | ||
| "S8924", | ||
| "S8947", | ||
| "S8948" | ||
| ] | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.