DialogResourceProviderImpl throws StringIndexOutOfBoundsException

[joerghoh]

Required Information

• AEM Version, including Service Packs, Cumulative Fix Packs, etc: 6.5.2
• ACS AEM Commons Version: latest
• Reproducible on Latest? yes

I have ticked the "enabled" checkbox for the PID com.adobe.acs.commons.mcp.impl.DialogResourceProviderFactoryImpl and now I get periodically this exception in the logs:

24.05.2020 18:11:05.405 *DEBUG* [sling-default-4-health-com.adobe.granite.repository.hc.impl.content.sling.SlingContentHealthCheck] com.adobe.acs.commons.mcp.impl.DialogResourceProviderImpl Get resource at path: /apps/unknown/type/cq:dialog
24.05.2020 18:11:05.405 *ERROR* [sling-default-4-health-com.adobe.granite.repository.hc.impl.content.sling.SlingContentHealthCheck] com.adobe.granite.queries.impl.hc.QueryHealthCheckMetrics Error in refreshing the health check gauge
java.lang.StringIndexOutOfBoundsException: String index out of range: -2
        at java.lang.String.substring(String.java:1931)
        at com.adobe.acs.commons.mcp.impl.DialogResourceProviderImpl.getResource(DialogResourceProviderImpl.java:113) [com.adobe.acs.acs-aem-commons-bundle:4.7.1.SNAPSHOT]
        at org.apache.sling.resourceresolver.impl.providers.stateful.AuthenticatedResourceProvider.getResource(AuthenticatedResourceProvider.java:135) [org.apache.sling.resourceresolver:1.6.8]
        at org.apache.sling.resourceresolver.impl.helper.ResourceResolverControl.listChildren(ResourceResolverControl.java:298) [org.apache.sling.resourceresolver:1.6.8]
        at org.apache.sling.resourceresolver.impl.ResourceResolverImpl.listChildren(ResourceResolverImpl.java:518) [org.apache.sling.resourceresolver:1.6.8]
        at org.apache.sling.resourceresolver.impl.ResourceResolverImpl.hasChildren(ResourceResolverImpl.java:1005) [org.apache.sling.resourceresolver:1.6.8]
        at org.apache.sling.api.resource.AbstractResource.hasChildren(AbstractResource.java:111) [org.apache.sling.api:2.20.0]
        at com.adobe.granite.repository.hc.impl.content.sling.SearchPathResourceBrowser.traverseAllResource(SearchPathResourceBrowser.java:61) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.SearchPathResourceBrowser.traverseAllResource(SearchPathResourceBrowser.java:62) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.SearchPathResourceBrowser.browseResources(SearchPathResourceBrowser.java:54) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.SearchPathResourceBrowser.accept(SearchPathResourceBrowser.java:48) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.SearchPathResourceBrowser.accept(SearchPathResourceBrowser.java:29) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.CompositeSearchPathResourceBrowser.accept(CompositeSearchPathResourceBrowser.java:37) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.SlingContentHealthCheck.checkSlingContent(SlingContentHealthCheck.java:140) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.SlingContentHealthCheck.executeContentCheck(SlingContentHealthCheck.java:105) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.repository.hc.impl.content.sling.SlingContentHealthCheck.execute(SlingContentHealthCheck.java:98) [com.adobe.granite.repository.hc.impl:1.0.58]
        at com.adobe.granite.queries.impl.hc.QueryHealthCheckMetrics$LazyGauge.execute(QueryHealthCheckMetrics.java:266) [com.adobe.granite.queries:1.0.64]
        at com.adobe.granite.queries.impl.hc.QueryHealthCheckMetrics$LazyGauge.lambda$new$0(QueryHealthCheckMetrics.java:253) [com.adobe.granite.queries:1.0.64]
        at org.apache.sling.commons.scheduler.impl.QuartzJobExecutor.execute(QuartzJobExecutor.java:347) [org.apache.sling.commons.scheduler:2.7.2]
        at org.quartz.core.JobRunShell.run(JobRunShell.java:202) [org.apache.sling.commons.scheduler:2.7.2]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)

This AEM instance is pretty much ootb (next to ACS AEM Commons).
I also haven't found any message which should be printed on DEBUG level when there were problems with evaluating the resourcetype of a sling model (see

acs-aem-commons/bundle/src/main/java/com/adobe/acs/commons/mcp/impl/DialogResourceProviderImpl.java

Line 98 in 7f8eb08

LOGGER.debug("Unable to determine sling resource type for model bean: {} ", originalClass);

)

[joerghoh]

Found by some debugging, that the problematic slingmodel is of class com.adobe.acs.commons.indesign.dynamicdeckdynamo.models.DynamicDeckInitiatorPageModel

[joerghoh]

The problem seems to be that this slingModel does neither specify a resourceType in the @model annotation nor as a property, thus the default /unknown/type is assumed, which fails.

@badvision How can we handle this best? I am not really deep into this feature at all, so I wonder if we should rather change this feature to require a opt-in from each SlingModel which wants to use this feature.

[davidjgonzalez]

Yeah - this impl is too aggressive. It seems like like a a better paradigm would’ve been:

1. only explicitly registered RTs are Bound
2. the model must sub-resource-type some specific RT
3. the models RT has to have a cq:dialog node in an expected location

If we need to add a breaking change to to this to prevent it from breaking general use, we should roll it into 5.0.0

Do we like one of the above options best? And/or are there others? I like #2.

[joerghoh]

I would opt for variant of 1: A sling model must provide an extra property to be considered by the DialogResourceProvider (plus of course providing the model property in the @model annotation). Option 2 limits the flexibility and also the Sling Models do not know the concept of ResourceSuperTypes as the resource resolution does.
Regarding option 3 I assume that it might confuse people finding that node in CRXDE but no dialog definition (although a dialog is present).

In any case the case of a fallback RT (like /unknown/type today) must be removed to avoid the mentioned problem

[davidjgonzalez]

The drawback with #1 is the user of this feature has to know/manage what is included, so if new SM's are added, they have to know to manage that list. Either way is fine with me, but as a user, i would just want all SlingModels that are built for this purpose to be automatically made available to me. sling:resourceSuperType might not be the right hook; but i wonder if there could be another way to self-identify; perhaps they have to register against some specific interface?

[badvision]

I agree we need a breaking change to fix the dialog resource provider, but the issue is that sling models aren't held in a registry that lends itself to interrogation easily. Therefore the more prudent solution is to change the @DialogProvider annotation to be a compiler-side annotation and introduce a processor which decorates the model to implement a proper service interface, etc. I can then remove the feature flag mess as well and bind to only a specific OSGi service interface.

[badvision]

OK the new implementation simply does not register resource providers if the resource type is not known. This is a more sensible approach overall. There were other performance issues unreported, but nonetheless present, with the previous dialog provider factory and those were resolved with this change as well.