Skip to content

Conversation

@rombirli
Copy link
Contributor

@rombirli rombirli commented Dec 23, 2025

Important notes

As noted in Gabriel's review, this PR will trim a few True Positives (TPs) in specific scenarios like the one below:

<string name="lipsum">Lorem ipsum dolor, sit ame
conesctetur</string> 

Current Context & Rationale

Currently, an issue is reported on the closing tag (</string>) for the wrong reason.

  • The "Actual" Reason: SonarXML raises an issue on the closing tag simply because it miscalculates the indentation based on the number of characters preceding the tag on that same line.
  • The "Expected" Reason: If we were to report this properly (as Gabriel suggested), the issue should actually be on the second line of the content, because the line continuation should be indented one level further than the opening tag.

Why I chose not to implement the "Proper Check"

While Gabriel proposed checking string content indentation properly, I have decided to keep string content ignored in IndentationCheck. I believe this remains the best option for the following reasons:

  1. Avoiding False Positives (FPs): While conventions exist for simple multiline strings (Line Continuations and Multiline Strings), XML is frequently used to wrap raw data or code snippets (like the example below). In these cases, standard indentation rules are often not relevant.
  2. Embedded Content Complexity: In scenarios like this, a "proper" indentation check would produce significant noise:
<bean id="bean1" class="org.abc.ClassA">
  <constructor-arg>
    <value>inline:
package org.abc;

class ClassA {
  int function1() {
      return 0;
  }
}
    </value>
  </constructor-arg>
</bean>

By maintaining the exclusion of string content from the check, and keeping the trivial check, we avoid reporting "accidental" issues on closing tags while preventing a high volume of False Positives in files containing embedded scripts or formatted text.

@hashicorp-vault-sonar-prod
Copy link

hashicorp-vault-sonar-prod bot commented Dec 23, 2025

SONARXML-180

@sonarqube-next
Copy link

@rombirli rombirli marked this pull request as ready for review December 26, 2025 10:11
@rombirli rombirli requested a review from a team December 26, 2025 10:11
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2026

This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically

@github-actions github-actions bot added the stale label Jan 3, 2026
@GabrielFleischer GabrielFleischer requested review from GabrielFleischer and removed request for GabrielFleischer January 5, 2026 12:49
Copy link
Contributor

@GabrielFleischer GabrielFleischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your method is a bit too broad and trim too many TPs.

I provided you with a moretest cases.

@github-actions github-actions bot removed the stale label Jan 6, 2026
@rombirli
Copy link
Contributor Author

rombirli commented Jan 7, 2026

I spent way too much time on it and it looks fixing it would be more complicated than initially expected.

@rombirli rombirli closed this Jan 7, 2026
@rombirli rombirli reopened this Jan 7, 2026
Revert "Draft of full solution not trimming too many FP"

This reverts commit b604297.

Revert "Refactoring : split test into MultilineString.xml and LineContinuation.xml"

This reverts commit 0c9cc3e.

Revert "Refactoring tests - use parametrized tests"

This reverts commit e10bfa4.

Revert "Refactoring - clean IndentationCheck.java"

This reverts commit 43af5f0.

Revert "Refactoring & behavior fix, add non-trivial tests"

This reverts commit 300ab87.

Revert "Fix qg"

This reverts commit 653a104.

Revert "Fix plugin test"

This reverts commit 4194663.

Revert "Continue small fixes and edge case tests"

This reverts commit 712666c.
Copy link
Contributor

@GabrielFleischer GabrielFleischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I join you on the fact that the lost FPs provide more value than the lost TPs.

So, LGTM

I still thing the comment explaining the exception would be better by using whitespaces rather than -, but this is nitpicky.

@rombirli
Copy link
Contributor Author

rombirli commented Jan 7, 2026

As discussed, I join you on the fact that the lost FPs provide more value than the lost TPs.

So, LGTM

I still thing the comment explaining the exception would be better by using whitespaces rather than -, but this is nitpicky.

good catch, I applied the suggestion

@sonarqube-next
Copy link

sonarqube-next bot commented Jan 7, 2026

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking to keep out of release 2.15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants