wttech / AEM-Rules-for-SonarQube

SonarQube plugin with set of rules detecting possible bugs and bad smells specific for AEM development.
Apache License 2.0
112 stars 51 forks source link

AEM-17 is claiming replace method is not mutable? #237

Open holdsworthr opened 1 year ago

holdsworthr commented 1 year ago

Hi

I'm trying to get rid of this code smell but it appears this sonar rule believes the replace method is not mutable

image

image

toniedzwiedz commented 1 year ago

Hi @holdsworthr, thanks for reporting this. Looking https://github.com/wttech/AEM-Rules-for-SonarQube/blob/master/src/main/java/com/cognifide/aemrules/java/checks/ModifiableValueMapUsageCheck.java#L63, the rule only recognises put, putAll and remove as mutable methods.

I'm guessing that the implementation follows the ModifiableValueMap Javadoc, which says:

The modifiable value map is only changeable through one of these methods

This is odd, because other methods inherited from java.util.Map have the effect of changing the values of the properties of the ValueMap's underlying resource.

Unless there's something I'm missing, both the AEM-17 Rule, and the Sling Javadoc could need a change. There may be a reason the Javadoc does not recommend using replace or replaceAll. If there's a good reason not to use those, we may be looking at a new rule. Let me search the Apache Sling Jira to find out if this has been discussed.

toniedzwiedz commented 1 year ago

https://issues.apache.org/jira/browse/SLING-11878 has been raised

toniedzwiedz commented 1 year ago

@holdsworthr it looks like these methods are valid. I think we'll improve AEM-17 to no longer report them. In the meantime, I would suggest resolving these issues as a won't fix or false positive on your Sonar.

toniedzwiedz commented 1 year ago

https://github.com/apache/sling-org-apache-sling-api/pull/48 has been merged. This means we should update the rule to allow the following methods in addition to the ones it currently supports: