SLING-13070 - ScriptContextProvider for bundled scripts does not properly set JAKARTA_REQUEST and JAKARTA_REPONSE

[rombert]

Use SlingBindings to ensure both javax and jakarta bindings are populated. Use the jakarta API variants to minimise wrapping.

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[enapps-enorman]

Can you provide the details or a sample on how to reproduce the error? I was under the impression that the SlingBindings would automatically provide the alternate wrapper for the jakarta or javax variants. But I may need to catch the error in a debugger to see what is happening.

[rombert]

Unfortunately I don't have a quick way of reproducing with the Sling Starter. It requires bundles/precompiled scripts which are used OOTB by AEM.

I am aware of https://adapt.to/2018/presentations/adaptto2018-apache-sling-scripting-reloaded-radu-cotescu-karl-pauls.pdf from @raducotescu and @karlpauls but I am not sure if there is a sample somewhere ready to be used. I found nothing in https://github.com/apache/sling-samples/ and only an old PR in ist-dresden/composum-nodes#206 .

[enapps-enorman]

Ok. Well conceptually, switching to using the SlingBindings should work to get the wrappers applied. But I wouldn't want to approve the changes without some way to verify it works in practice. Ideally some automated integration test to make sure it isn't broken again in the future. Especially since I have never used the precompiled scripts myself so I am not very familiar with the flow.

[rombert]

Ok. Well conceptually, switching to using the SlingBindings should work to get the wrappers applied. But I wouldn't want to approve the changes without some way to verify it works in practice. Ideally some automated integration test to make sure it isn't broken again in the future. Especially since I have never used the precompiled scripts myself so I am not very familiar with the flow.

Yes, that makes sense. I'll try and see if I can add a precompiled script to the starter tests.

[raducotescu]

We used to have extensive tests for bundled/precompiled scripts in https://github.com/apache/sling-org-apache-sling-scripting-bundle-tracker-it. Unfortunately, that module hasn't been maintained in a while (my fault too...), and it doesn't build with Java 17 or newer.

Given that we now ship support for bundled/precompiled scripts in our code modules, maybe we should move those tests into the starter?

[rombert]

Ok. Well conceptually, switching to using the SlingBindings should work to get the wrappers applied. But I wouldn't want to approve the changes without some way to verify it works in practice. Ideally some automated integration test to make sure it isn't broken again in the future. Especially since I have never used the precompiled scripts myself so I am not very familiar with the flow.

Yes, that makes sense. I'll try and see if I can add a precompiled script to the starter tests.

@enapps-enorman - I made the following changes:

• added bundled script support to the starter test services - SLING-13070 - ScriptContextProvider for bundled scripts does not properly set JAKARTA_REQUEST and JAKARTA_REPONSE sling-org-apache-sling-starter-test-services#22
• added tests for JSP and HTL bundled scripts to the starter integration tests - SLING-13070 - ScriptContextProvider for bundled scripts does not properly set JAKARTA_REQUEST and JAKARTA_REPONSE sling-org-apache-sling-starter-integration-tests#43

Those tests are not enough to trigger the failures. They only happen when the HTL scripting engine has the scriptResolutionCache enabled. I configured in a draft PR to demonstrate the problem - apache/sling-org-apache-sling-starter#579 . I don't think we should merge it as-is because there is no out-of-the-box issue; the test should be on a lower level, which I think this PR provides.

[rombert]

We used to have extensive tests for bundled/precompiled scripts in https://github.com/apache/sling-org-apache-sling-scripting-bundle-tracker-it. Unfortunately, that module hasn't been maintained in a while (my fault too...), and it doesn't build with Java 17 or newer.

Given that we now ship support for bundled/precompiled scripts in our code modules, maybe we should move those tests into the starter?

Thanks for the review @raducotescu .

I added a couple of basic tests ( see my reply to Eric ). For the extensive tests I think we should follow the approach that @stefanseifert took and include the ITs in the implementation module - see Move integration tests to impl project - sling models impl because they are closer to the code they yest.

[rombert]

I plan to merge this on Monday, in case anyone else wants to review.