SLING-12359 - Extend records support

[ciechanowiec]

Java 14 introduced support for record classes, and Apache Sling 12 by default runs with Java 17, allowing the use of records.

Although Apache Sling 12 supports record classes, the current latest version of Apache Sling Models Implementation does not fully support injection in relation to records. Specifically, for records, the Apache Sling Models Implementation does support constructor injection similarly to a usual class if there is an explicit constructor annotated with javax.inject.Inject:

@Model(
        adaptables = {Resource.class, SlingHttpServletRequest.class},
        defaultInjectionStrategy = DefaultInjectionStrategy.REQUIRED
)
public record StudentModel(String name, int age) {

    @Inject
    public StudentModel(
            @ValueMapValue(name = "name") @Default(values = "unknown")
            String name, 
            @ValueMapValue(name = "age") @Default(intValues = 0)
            int age
    ) {
        this.name = name;
        this.age = age;
    }
}

However, for records, Apache Sling Models Implementation does not support constructor injection if there is an implicit constructor. Such a constructor cannot be annotated with javax.inject.Inject and will not be picked up during injection. Therefore, such a Sling Model will not be instantiated at all:

@Model(
        adaptables = {Resource.class, SlingHttpServletRequest.class},
        defaultInjectionStrategy = DefaultInjectionStrategy.REQUIRED
)
public record StudentModel(
        @ValueMapValue(name = "name") @Default(values = "unknown")
        String name,
        @ValueMapValue(name = "age") @Default(intValues = 0)
        int age
) {}

This pull request addresses the above issue by introducing support for injection in records' implicit constructors.

Note:

1. For records, only constructor injection is supported since records cannot have instance fields (code with such fields will not compile).
2. Apache Sling Models Implementation is currently based on Java 8 and may also be executed in a Java 8 environment. Therefore, the records check is introduced via dynamic loading of the java.lang.Record class.

[rombert]

Thanks for the PR @ciechanowiec , happy to see further support added for records; in my own limited testing I did not run into this problem. My ask would be that you create a Jira for this enhancement as it's non-trivial (and impactful).

I think someone with more models knowledge should review this ( maybe @kwin / @stefanseifert ? ).

[ciechanowiec]

@rombert , thank you for your comment.

According to your suggestion, I've created a Jira ticket for this improvement and updated this pull request to reference that ticket:
https://issues.apache.org/jira/projects/SLING/issues/SLING-12359

[stefanseifert]

thanks for the contribution - this looks very good!

i've created an experimental branch with a Java 17 based unit test actually using this extension and a Record class: https://github.com/apache/sling-org-apache-sling-models-impl/tree/experimental/SLING-12359-record-unit-test

it works fine for named injections - but it does not work without explicitly defining the attribute names - in this case it should default to the property name from code, it should not be required to put a name in the annotation if it's the same. is this possible without relying on Java 17?

we do not have to stick with Java 8, next parent update will raise it to Java 11 as minimum dependency.

[ciechanowiec]

@stefanseifert, thank you for your valuable insights. Below is my response regarding the topic of named constructor parameters. I'll address the rest of your suggestions separately to keep this comment focused.

If I understand you correctly, you suggested that for unnamed parameters of record constructors (i.e., parameters without a @Named annotation and without a name element on injector-specific annotations), injection that requires a name for resolving should rely on the parameter names themselves as a fallback. Although this is a good idea, I'm not perfectly sure it should be implemented in this pull request for the reasons outlined below.

1. By default, the Java compiler does not preserve parameter names of constructors, so these names are not available for reflective access during runtime (see the official documentation: https://docs.oracle.com/javase/tutorial/reflect/member/methodparameterreflection.html).

2. According to the official Sling Models documentation, for unnamed parameters of regular class constructors (i.e., parameters without a @Named annotation and without a name element on injector-specific annotations), injection that requires a name for resolving does not rely on the parameter names themselves as a fallback. The documentation states that in such cases, a @Named annotation or a name element on injector-specific annotations is mandatory to resolve the injection (see https://sling.apache.org/documentation/bundles/models.html):

Because the name of a constructor argument parameter cannot be detected via the Java Reflection API, a @Named annotation (or a name element on injector-specific annotations) is mandatory for injectors that require a name for resolving the injection.

3. Enforcing mandatory names for parameters of regular class constructors that require a name for resolving the injection is currently implemented in unit tests. It is expected that in such cases, a Sling Model will not be instantiated at all:

   sling-org-apache-sling-models-impl/src/test/java/org/apache/sling/models/impl/ConstructorTest.java

   Lines 192 to 196 in 2312e13

   	@Test
   	public void testNoNameModel() {
   	NoNameModel model = factory.getAdapter(request, NoNameModel.class);
   	assertNull(model);
   	}

Therefore, if we introduce a rule that for unnamed parameters of record constructors (i.e., parameters without a @Named annotation and without a name element on injector-specific annotations) injection that requires a name for resolving should rely on the parameter names themselves as a fallback, this behavior:

a) would be applied inconsistently and only in cases where compilation was performed in a non-standard way, and

b) would not comply with the current and expected Sling Models behavior for regular classes.

Due to these reasons, I conclude that names for parameters of constructors that require a name for resolving the injection should be mandatory not only for regular classes (as currently implemented) but also for record classes. There should be no fallback to the record constructor parameter names themselves, just as there is no such fallback in case of regular classes. This pull request adheres to this conclusion and introduces the same behavior for record classes as is already implemented for regular classes.

Please let me know if the pull request requires any changes regarding named constructor parameters, or if we can proceed as it is. I appreciate your guidance and look forward to your feedback.

[stefanseifert]

Please let me know if the pull request requires any changes regarding named constructor parameters, or if we can proceed as it is. I appreciate your guidance and look forward to your feedback.

you're right! thanks for the elaborate answer. that's not supported currently in sling models for any constructors currently.

actually, there was a change in Java 8 to better get access to constructor parameters and their names and we have a SLING-11917 plus PR #45 pending for it - but it was not yet applied (and it only works with a special compiler option).

so this is out of scope for this PR.

[stefanseifert]

looks good!

cosmetic: can you try to get rid of the two new sonar issues (although they apply only to unit test code): https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=51&resolved=false&sinceLeakPeriod=true

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud