search

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

Feature: Create rule to enforce that adaptable is not stored inside model #234

Open kwin opened 1 year ago

kwin commented 1 year ago

According to https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector the adaptable should not be stored in the model when cache=true is used, as this might exhaust the memory easily. A rule to enforce that the adaptable is not stored in a field when the adaptable has cache=true should be added to prevent this.

toniedzwiedz commented 1 year ago

Hi @kwin, sorry for the late response, I missed it amidst the holiday/end-of-year craze. This does look like a good candidate for a rule.

I've got a couple of follow-up questions to better determine what the rule should look like.

  1. Is this only an issue when the @Self injector is used? Or is it enough to inject the underlying SlingHttpServletRequest?
  2. It seems the issue has been resolved https://github.com/apache/sling-org-apache-sling-models-impl/pull/9/commits/5b472557cc6e6057e781a4bf0e65ddfe3d3e8430 so a rule would only be relevant to versions of Apache Sling that don't have the fix in place. Is this a correct interpretation?
kwin commented 1 year ago
  • Is this only an issue when the @Self injector is used? Or is it enough to inject the underlying SlingHttpServletRequest?

Any means to inject and store the adaptable in the model should be prevented. That also affects adaptable Resource (although to a lesser degree)

Although with that commit, heap space exhaustion is prevented, this still leads to unexpected behaviour. Compare with https://sling.apache.org/documentation/bundles/models.html#a-note-about-cache-true-and-using-the-self-injector

Starting in version 1.4.10, storing the original adaptable will not crash the JVM, but it can cause unexpected behavior (e.g. a model being created twice, when it should be cached). The issue was first reported in SLING-7586.