SONARPHP-1816 fix: resolve 5 SonarQube issues with visibility and singleton pattern#1698
SONARPHP-1816 fix: resolve 5 SonarQube issues with visibility and singleton pattern#1698sonarqube-agent[bot] wants to merge 1 commit into
Conversation
Fixed issues: - AZkoVAGjIsbR56mqmhMT for java:S5786 rule - AZkoVAM1IsbR56mqmhMt for java:S5786 rule - AZkoU_MlIsbR56mqmhHL for java:S5786 rule - AZkoVACCIsbR56mqmhL6 for java:S6548 rule - AZkoVACgIsbR56mqmhL8 for java:S6548 rule Generated by SonarQube Agent (task: 76ff4569-725c-4b15-bafc-970165160f40)
hashicorp-vault-sonar-prod
Bot
changed the title
|
Summary
This PR fixes 5 SonarQube code quality issues through straightforward refactoring: Test visibility (3 files): Removed Enum-to-utility class (2 files): Converted UnknownLocationInFile and BuiltinSymbolData from enum singletons to final classes with static fields/methods. The enum pattern was unnecessary for these utility-style classes. All changes are safe refactoring with no behavioral impact—the static instances and methods provide identical access patterns. What reviewers should knowKey files to review:
What to verify: Check that the enum-to-class conversions don't break any other callers of these classes. BuiltinSymbolData.BUILTINS and UnknownLocationInFile.UNKNOWN_LOCATION should have been the only usage points (and they're updated here).
|
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
There was a problem hiding this comment.
The test-visibility changes are correct JUnit 5 idiom and the BuiltinSymbolData refactor is clean. However, the UnknownLocationInFile conversion introduces a subtle but real bug: the constructor is package-private instead of private, which breaks the singleton guarantee that the rest of the codebase relies on.
|
|
||
| UnknownLocationInFile() { | ||
| } |
There was a problem hiding this comment.
The constructor is package-private, not private. This breaks the singleton guarantee.
PreciseIssue.java uses identity comparison (==) against UnknownLocationInFile.UNKNOWN_LOCATION, so correctness depends on there being exactly one instance. With a package-private constructor, any class in the org.sonar.php.symbols package (e.g. a future SymbolFactory) can call new UnknownLocationInFile() and produce a second instance that is ==-unequal to UNKNOWN_LOCATION, silently causing PreciseIssue.secondary() to accept locations it should skip.
The enum pattern gave this for free; the manual conversion must replicate it explicitly.
| UnknownLocationInFile() { | |
| } | |
| private UnknownLocationInFile() { | |
| } |
- Mark as noise
|
SonarQube Remediation Agent could not fix the CI failures. Task: CI failure fix failed: No CI logs could be downloaded for any failed check run; aborting fix attempt. Generated by SonarQube Remediation Agent |
|
SonarQube Remediation Agent could not fix the CI failures. Task: CI failure fix failed: No CI logs could be downloaded for any failed check run; aborting fix attempt. Generated by SonarQube Remediation Agent |
|
Changes seem to be partially not necessary. In particular, the enum for |
1 participant
Removes unnecessary 'public' modifiers from JUnit5 lifecycle methods in three test classes and converts two enum-based Singletons to final utility classes. These changes improve code quality by following JUnit5 conventions and eliminating the enum Singleton pattern where it was unnecessary.
View Project in SonarCloud
Fixed Issues
java:S5786 - Remove this 'public' modifier. • INFO • View issue
Location:
php:php-checks/src/test/java/org/sonar/php/checks/NonLFCharAsEOLCheckTest.java:41Why is this an issue?
JUnit5 is more tolerant regarding the visibility of test classes and methods than JUnit4, which required everything to be
public. Test classes and methods can have any visibility exceptprivate. It is however recommended to use the default package visibility to improve readability.What changed
This hunk removes the 'public' modifier from the setUp() lifecycle method in the JUnit5 test class NonLFCharAsEOLCheckTest, changing it from 'public void setUp()' to 'void setUp()'. JUnit5 test classes and lifecycle methods should use default package visibility rather than 'public', as JUnit5 does not require public visibility. This directly addresses the code smell about unnecessary 'public' modifier on a JUnit5 lifecycle method at line 41 of NonLFCharAsEOLCheckTest.java.
java:S5786 - Remove this 'public' modifier. • INFO • View issue
Location:
php:php-frontend/src/test/java/org/sonar/php/tree/visitors/PHPVisitorCheckTest.java:66Why is this an issue?
JUnit5 is more tolerant regarding the visibility of test classes and methods than JUnit4, which required everything to be
public. Test classes and methods can have any visibility exceptprivate. It is however recommended to use the default package visibility to improve readability.What changed
This hunk removes the 'public' modifier from the setUp() lifecycle method in the JUnit5 test class PHPVisitorCheckTest. JUnit5 test classes and methods should use default package visibility instead of 'public' for better readability and convention compliance. By changing 'public void setUp()' to 'void setUp()', the unnecessary 'public' modifier is removed, resolving the code smell.
java:S6548 - An Enum-based Singleton implementation was detected. Make sure the use of the Singleton pattern is required and an Enum-based implementation is the right one for the context. • INFO • View issue
Location:
php:php-frontend/src/main/java/org/sonar/php/symbols/BuiltinSymbolData.java:32Why is this an issue?
While the Singleton pattern can be useful in certain situations, overusing it can have several drawbacks:
What changed
Changes the BuiltinSymbolData from an enum to a final class, directly eliminating the Enum-based Singleton pattern that was flagged by the static analysis rule java:S6548. This removes the Singleton design pattern entirely by converting to a utility class with static methods.
java:S6548 - An Enum-based Singleton implementation was detected. Make sure the use of the Singleton pattern is required and an Enum-based implementation is the right one for the context. • INFO • View issue
Location:
php:php-frontend/src/main/java/org/sonar/php/symbols/UnknownLocationInFile.java:21Why is this an issue?
While the Singleton pattern can be useful in certain situations, overusing it can have several drawbacks:
What changed
This hunk converts the UnknownLocationInFile from an enum to a final class, directly addressing the static analysis warning about an Enum-based Singleton implementation. By changing from 'public enum UnknownLocationInFile' to 'public final class UnknownLocationInFile', the code no longer uses the Enum Singleton pattern that was flagged by the scanner.
java:S5786 - Remove this 'public' modifier. • INFO • View issue
Location:
php:php-frontend/src/test/java/org/sonar/php/highlighter/SymbolHighlighterTest.java:54Why is this an issue?
JUnit5 is more tolerant regarding the visibility of test classes and methods than JUnit4, which required everything to be
public. Test classes and methods can have any visibility exceptprivate. It is however recommended to use the default package visibility to improve readability.What changed
This hunk removes the 'public' modifier from the setUp() lifecycle method in the JUnit5 test class SymbolHighlighterTest. JUnit5 test classes and lifecycle methods should use default package visibility rather than 'public' for better readability and convention compliance. By changing 'public void setUp()' to 'void setUp()', the unnecessary 'public' modifier is removed as recommended by the JUnit5 User Guide.
SonarQube Remediation Agent uses AI. Check for mistakes.