Remove private constructors#455
Merged
madurangasiriwardena merged 1 commit intowso2:masterfrom Mar 4, 2026
Merged
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
54
to
56
| public static SCIMUserSchemaExtensionBuilder getInstance() { | ||
| return configReader; | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public static SCIMUserSchemaExtensionBuilder getInstance() { | |
| return configReader; | |
| } | |
| public static SCIMUserSchemaExtensionBuilder getInstance() { | |
| log.debug("Getting SCIMUserSchemaExtensionBuilder instance"); | |
| return configReader; | |
| } |
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 |
WalkthroughRemoved private constructors from four singleton-like classes in the configuration and schema modules, exposing default (package-private or public) constructors instead. Added a SpotBugs exclusion to suppress warnings about non-private singleton constructors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spotbugs-exclude.xml`:
- Around line 29-31: The SpotBugs suppression currently disables
SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR globally; instead, narrow the filter
entries in spotbugs-exclude.xml to target only the four classes changed in this
PR: CharonConfiguration, SCIMSystemSchemaExtensionBuilder,
SCIMUserSchemaExtensionBuilder, and SCIMResourceSchemaManager. Update the
<Match> section so each class is specified (by fully qualified class name) with
a Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" entry (or a single
Match with multiple <Class name="..."/> children) so the suppression applies
only to those four classes and not the entire codebase. Ensure the class names
match their package-qualified names used in the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dbca5b0-a309-477d-be1b-9ebd4aa4dca1
📒 Files selected for processing (5)
modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.javamodules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.javamodules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.javamodules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.javaspotbugs-exclude.xml
💤 Files with no reviewable changes (4)
- modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java
- modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java
- modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java
- modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.java
Comment on lines
+29
to
+31
| <Match> | ||
| <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" /> | ||
| </Match> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the spotbugs-exclude.xml file to see the current suppression
cat -n spotbugs-exclude.xml | head -40Repository: wso2/charon
Length of output: 1486
🏁 Script executed:
# Check the specific lines mentioned in the review
sed -n '25,35p' spotbugs-exclude.xmlRepository: wso2/charon
Length of output: 298
🏁 Script executed:
# Search for all SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR suppressions
rg -n 'SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR' spotbugs-exclude.xml -B2 -A2Repository: wso2/charon
Length of output: 168
🏁 Script executed:
# Find Java files that were likely modified in this PR (those with singleton pattern changes)
# Look for classes that have getInstance() methods and constructors
fd --type f --name "*.java" modules | head -20Repository: wso2/charon
Length of output: 305
🏁 Script executed:
# Check the suggested classes to see if they exist and their constructor visibility
for class_path in \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.java"
do
echo "=== Checking $class_path ==="
if [ -f "$class_path" ]; then
rg -n 'class |getInstance|private|public|protected' "$class_path" | head -15
else
echo "File not found"
fi
doneRepository: wso2/charon
Length of output: 4875
🏁 Script executed:
# Check CharonConfiguration constructor
rg -n 'public|private|protected' modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java | grep -i "CharonConfiguration\s*("Repository: wso2/charon
Length of output: 145
🏁 Script executed:
# Check SCIMSystemSchemaExtensionBuilder constructor
rg -n 'public|private|protected' modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java | grep -i "SCIMSystemSchemaExtensionBuilder\s*("Repository: wso2/charon
Length of output: 166
🏁 Script executed:
# Check SCIMUserSchemaExtensionBuilder constructor
rg -n 'public|private|protected' modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java | grep -i "SCIMUserSchemaExtensionBuilder\s*("Repository: wso2/charon
Length of output: 160
🏁 Script executed:
# Check SCIMResourceSchemaManager constructor
rg -n 'public|private|protected' modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.java | grep -i "SCIMResourceSchemaManager\s*("Repository: wso2/charon
Length of output: 145
🏁 Script executed:
# Use a more direct approach - check for explicit constructor declarations in these files
for file in \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.java"
do
echo "=== $file ==="
rg -n '\s+(public|private|protected)\s+\w+\s*\(\s*\)' "$file" | head -5
doneRepository: wso2/charon
Length of output: 468
🏁 Script executed:
# Check each file for all constructor declarations (explicit or implicit)
for file in \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.java"
do
echo "=== $(basename $file) ==="
class_name=$(basename "$file" .java)
# Look for constructor with class name
rg -n "$class_name\s*\(" "$file" | head -10
doneRepository: wso2/charon
Length of output: 616
🏁 Script executed:
# Get full file content to check constructor declarations for CharonConfiguration
cat modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java | head -80Repository: wso2/charon
Length of output: 2437
🏁 Script executed:
# Get full file content to check constructor declarations for SCIMSystemSchemaExtensionBuilder
cat modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java | head -100Repository: wso2/charon
Length of output: 3834
🏁 Script executed:
# Check if any of these classes were modified in recent commits
git log --oneline --name-only -20 | head -50Repository: wso2/charon
Length of output: 3478
🏁 Script executed:
# Get the full list of changed files in the "Remove private constructors" commit
git show --name-only da5ce5b | grep -E "\.java$" | head -50Repository: wso2/charon
Length of output: 4181
🏁 Script executed:
# Get more detail on what changed in the key files
git show da5ce5b -- modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java | head -100Repository: wso2/charon
Length of output: 3249
🏁 Script executed:
# Check if there are other classes in the codebase with non-private constructors that might trigger this warning
rg -l 'getInstance\s*\(' --type java modules | xargs -I {} sh -c 'rg -n "class |getInstance|private|public" {} | head -5 && echo "---"' | head -100Repository: wso2/charon
Length of output: 6353
🏁 Script executed:
# Get the full diff for the commit to see which classes had constructors changed
git show da5ce5b --stat | grep "\.java$"Repository: wso2/charon
Length of output: 37
🏁 Script executed:
# Check if the classes mentioned in the review actually had private constructors removed
git show da5ce5b -- modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java | grep -A5 -B5 "public\|private" | head -80Repository: wso2/charon
Length of output: 3243
🏁 Script executed:
# Search for explicit constructor declarations in all modified files
git diff da5ce5b~1..da5ce5b --name-only | while read file; do
if [[ "$file" == *.java ]]; then
echo "=== $file ==="
git show da5ce5b:"$file" 2>/dev/null | grep -n "^\s*\(public\|private\|protected\)\s*$(basename "$file" .java)\s*(" || echo "No explicit constructor found"
fi
done | head -100Repository: wso2/charon
Length of output: 253
🏁 Script executed:
# Check all classes in the entire repository to find which ones have getInstance() with package-private constructors
find modules -name "*.java" -type f | while read file; do
if grep -q "getInstance()" "$file"; then
has_private=$(grep -c "private\s*$(basename "$file" .java)\s*(" "$file" 2>/dev/null || echo 0)
has_public=$(grep -c "public\s*$(basename "$file" .java)\s*(" "$file" 2>/dev/null || echo 0)
has_protected=$(grep -c "protected\s*$(basename "$file" .java)\s*(" "$file" 2>/dev/null || echo 0)
has_default=$(grep -c "^\s*$(basename "$file" .java)\s*(" "$file" 2>/dev/null || echo 0)
if [ "$has_private" -eq 0 ] && [ "$has_public" -eq 0 ] && [ "$has_protected" -eq 0 ]; then
echo "PACKAGE-PRIVATE: $file"
fi
fi
done | head -20Repository: wso2/charon
Length of output: 1704
🏁 Script executed:
# Get the full list of files changed in the commit
git log --oneline -1 da5ce5b
git show da5ce5b --name-only --pretty=format: | grep "\.java$" | sortRepository: wso2/charon
Length of output: 16660
🏁 Script executed:
# Check if these classes have explicit constructor definitions or rely on default constructors
for file in \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.java"
do
class_name=$(basename "$file" .java)
echo "=== $class_name ==="
# Search for explicit constructor declaration
if grep -q "$class_name\s*(" "$file"; then
grep -n "$class_name\s*(" "$file" | head -5
else
echo "No explicit constructor found - uses default/package-private"
fi
echo
doneRepository: wso2/charon
Length of output: 600
🏁 Script executed:
# Verify if the default constructor approach (no private) could cause issues
# by checking how many singleton-like classes exist in the codebase
echo "Classes with getInstance() method:"
rg -l "getInstance\s*\(\)" --type java modules | wc -l
echo
echo "Sample of classes with getInstance():"
rg -l "getInstance\s*\(\)" --type java modules | head -10Repository: wso2/charon
Length of output: 1111
🏁 Script executed:
# Check the three classes mentioned as still having private constructors
for file in \
"modules/charon-utils/src/main/java/org/wso2/charon3/utils/DefaultCharonManager.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMCustomSchemaExtensionBuilder.java" \
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMAgentSchemaExtensionBuilder.java"
do
echo "=== $(basename $file) ==="
class_name=$(basename "$file" .java)
if grep -q "private\s*$class_name\s*(" "$file" 2>/dev/null; then
echo "HAS private constructor"
grep -n "private\s*$class_name\s*(" "$file"
else
echo "NO private constructor found"
fi
# Also show getInstance if it has one
if grep -q "getInstance" "$file"; then
echo "Has getInstance method"
fi
echo
doneRepository: wso2/charon
Length of output: 489
🏁 Script executed:
# List all the specific classes that were modified in this commit and have constructors changed
git show da5ce5b --pretty=format: --name-only | grep -E "(SCIMCustomSchemaExtensionBuilder|SCIMAgentSchemaExtensionBuilder|CharonConfiguration|SCIMSystemSchemaExtensionBuilder|SCIMUserSchemaExtensionBuilder|SCIMResourceSchemaManager)\.java"Repository: wso2/charon
Length of output: 637
🏁 Script executed:
# Check if SCIMCustomSchemaExtensionBuilder and SCIMAgentSchemaExtensionBuilder were actually modified to remove private constructors
# or if they still have them
git show da5ce5b -- modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMCustomSchemaExtensionBuilder.java | grep -A10 -B10 "private\|public" | head -60Repository: wso2/charon
Length of output: 1886
🏁 Script executed:
# Check the diff more carefully to see what changed with SCIMAgentSchemaExtensionBuilder
git show da5ce5b -- modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMAgentSchemaExtensionBuilder.java | grep -C15 "SchemaExtensionBuilder\s*(" | head -80Repository: wso2/charon
Length of output: 1153
🏁 Script executed:
# Get a clearer picture: check current state vs what the review claims
# The review says these 6 classes were modified to remove private constructors
# Let's verify the current constructor visibility for all 6
python3 << 'EOF'
import subprocess
import re
files = [
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/CharonConfiguration.java",
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMAgentSchemaExtensionBuilder.java",
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMCustomSchemaExtensionBuilder.java",
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMSystemSchemaExtensionBuilder.java",
"modules/charon-core/src/main/java/org/wso2/charon3/core/config/SCIMUserSchemaExtensionBuilder.java",
"modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMResourceSchemaManager.java"
]
for filepath in files:
class_name = filepath.split("/")[-1].replace(".java", "")
print(f"\n=== {class_name} ===")
try:
with open(filepath, 'r') as f:
content = f.read()
# Look for constructor declarations
pattern = r'(public|private|protected)?\s+' + class_name + r'\s*\('
matches = re.finditer(pattern, content)
found = False
for match in matches:
start = max(0, match.start() - 100)
end = min(len(content), match.end() + 200)
print(f"Found: {content[match.start():match.end()]}")
print(f"Context: ...{content[start:end]}...")
found = True
if not found:
print("No explicit constructor found - uses default/package-private")
except FileNotFoundError:
print("File not found")
EOFRepository: wso2/charon
Length of output: 3399
Scope this SpotBugs suppression to the specific classes modified, not globally.
The suppression at lines 29-31 disables SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR for the entire codebase. This can mask regressions if future singleton classes are introduced with non-private constructors. Four classes in this PR had their private constructors removed and now use package-private constructors: CharonConfiguration, SCIMSystemSchemaExtensionBuilder, SCIMUserSchemaExtensionBuilder, and SCIMResourceSchemaManager. Restrict the exclusion to only these classes.
Proposed scoped filter update
- <Match>
- <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" />
- </Match>
+ <Match>
+ <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" />
+ <Class name="org.wso2.charon3.core.config.CharonConfiguration" />
+ </Match>
+ <Match>
+ <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" />
+ <Class name="org.wso2.charon3.core.config.SCIMSystemSchemaExtensionBuilder" />
+ </Match>
+ <Match>
+ <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" />
+ <Class name="org.wso2.charon3.core.config.SCIMUserSchemaExtensionBuilder" />
+ </Match>
+ <Match>
+ <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" />
+ <Class name="org.wso2.charon3.core.schema.SCIMResourceSchemaManager" />
+ </Match>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Match> | |
| <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" /> | |
| </Match> | |
| <Match> | |
| <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" /> | |
| <Class name="org.wso2.charon3.core.config.CharonConfiguration" /> | |
| </Match> | |
| <Match> | |
| <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" /> | |
| <Class name="org.wso2.charon3.core.config.SCIMSystemSchemaExtensionBuilder" /> | |
| </Match> | |
| <Match> | |
| <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" /> | |
| <Class name="org.wso2.charon3.core.config.SCIMUserSchemaExtensionBuilder" /> | |
| </Match> | |
| <Match> | |
| <Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" /> | |
| <Class name="org.wso2.charon3.core.schema.SCIMResourceSchemaManager" /> | |
| </Match> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spotbugs-exclude.xml` around lines 29 - 31, The SpotBugs suppression
currently disables SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR globally; instead,
narrow the filter entries in spotbugs-exclude.xml to target only the four
classes changed in this PR: CharonConfiguration,
SCIMSystemSchemaExtensionBuilder, SCIMUserSchemaExtensionBuilder, and
SCIMResourceSchemaManager. Update the <Match> section so each class is specified
(by fully qualified class name) with a Bug
pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR" entry (or a single Match
with multiple <Class name="..."/> children) so the suppression applies only to
those four classes and not the entire codebase. Ensure the class names match
their package-qualified names used in the codebase.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #455 +/- ##
============================================
- Coverage 31.78% 31.77% -0.02%
- Complexity 1114 1115 +1
============================================
Files 137 137
Lines 13553 13551 -2
Branches 2599 2599
============================================
- Hits 4308 4306 -2
Misses 8713 8713
Partials 532 532
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
madurangasiriwardena
approved these changes
Mar 4, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Purpose
Goals
Approach
User stories
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit