HSEARCH-4673 Use record specific methods to get constructor parameter names when applicable

[marko-bekhta]

https://hibernate.atlassian.net/browse/HSEARCH-4673

Hey Yoann! It's been a while 😑😃. I noticed this task on reflection, so I thought I'd look into it. From what I understand, it all starts inside ProjectionConstructorProcessor#process(), which led me to these SPI HCAnn classes...

As these are SPI, I'm not sure where it would be better to put the utils for records and if it's ok to have two implementations for the constructor or if we should keep just one and hide the logic inside...

[yrodiere]

Hey Marko! Thanks for that PR :)

Unfortunately... I think it would have been better to start with a test.

Looking at your code, I had a hunch and asked my self "hey, but since the compiler adds metadata for record components, maybe it also adds the param name to Parameter#getName(), just for records, even when not using the -parameters compiler flag? And indeed, it looks like it does: https://github.com/yrodiere/jdk-playground/blob/records-parameters/src/test/java/org/hibernate/jdk/playground/MyTest.java#L13-L14

So... The good news is that projection constructors for records compiled without using -parameters probably already work in Hibernate Search. The bad news is that changes in this PR won't be needed... They were still useful though, since they made me ask the right question and find out the answer :)

What's needed, though, is some testing and adjustments to the warnings about -parameters in the documentation.

Regarding testing, it's going to be a bit complex... We need to make sure that records involved in this test are compiled without the -parameters flag, and all test code is compiled with this flag...

So, if you're still up for it, I'd suggest:

• Adding a source directory java17-noparameters to integrationtest/mapper/pojo-base
• Adding a dedicated execution of the maven-compiler-plugin to that same module, making sure that -parameters is NOT enabled
• Copy-pasting ProjectionConstructorRecordIT to java17-noparameters (renaming it to ProjectionConstructorRecordNoParametersCompilerFlagIT?)
• Adding a test to ProjectionConstructorRecordNoParametersCompilerFlagIT to check that -parameters is indeed not enabled. Maybe just add a dummy class to the same source directory and check that its constructor parameters don't have a name (!param.isNamePresent())?

And if you work on this, don't forget to update the documentation, of course :)

Thanks again!

[marko-bekhta]

No worries 😃 I’ll make the changes.

[marko-bekhta]

sorry that it took me ages to go back to this PR 🙊

I have removed the constructorLevelAnnotation_nonCanonical case from the no-parameter-compiler-flag test as it fails without a compiler flag.

I also thought if we want to keep the additional sources folder as we can configure the compiler plugin with includes/excludes.

I also noticed that the build failed for the compiler-eclipse ... so I'll try to see what I can do about it.

[yrodiere]

I also thought if we want to keep the additional sources folder as we can configure the compiler plugin with includes/excludes.

We need different configuration (compiler flags) for each source folder, so we need multiple executions... not sure what you're suggesting?

I also noticed that the build failed for the compiler-eclipse ... so I'll try to see what I can do about it.

Looks like the main build is failing as well :)

[marko-bekhta]

let's see if the build works now 🙈

I have a bit more time to actually explain what I meant 😃 hopefully, it'll make sense 😉

We need different configuration (compiler flags) for each source folder, so we need multiple executions... not sure what you're suggesting?

If I get it correctly... adding test sources happens before their compilation, which means once we reach the step of compiling the tests, all of them are pulled into one folder, and it doesn't really matter for the compiler plugin where they came from.
That's why I've used a combination of testExcludes and testIncludes to filter what execution compiles which classes.

These testExcludes and testIncludes have a small problem... if the testIncludes will match no classes, it then decides to pick everything and all test sources get recompiled:

[INFO] --- maven-compiler-plugin:3.10.1:testCompile (default-testCompile) @ hibernate-search-documentation ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 372 source files to /mnt/jenkins-workdir/workspace/hibernate-search_PR-3240/documentation/target/test-classes
[INFO] 
[INFO] --- maven-compiler-plugin:3.10.1:testCompile (noparameters-testCompile) @ hibernate-search-documentation ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 372 source files to /mnt/jenkins-workdir/workspace/hibernate-search_PR-3240/documentation/target/test-classes

that's what caused the failure of main build -_-

but if there's a match, everything is ok and only a single test is compiled as expected:

[INFO] --- maven-compiler-plugin:3.10.1:testCompile (default-testCompile) @ hibernate-search-integrationtest-mapper-pojo-base ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 214 source files to /home/marko/development/projects/opensource/hibernate-search/integrationtest/mapper/pojo-base/target/test-classes
[INFO] 
[INFO] --- maven-compiler-plugin:3.10.1:testCompile (noparameters-testCompile) @ hibernate-search-integrationtest-mapper-pojo-base ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 1 source file to /home/marko/development/projects/opensource/hibernate-search/integrationtest/mapper/pojo-base/target/test-classes

To make it work, I've added a configurable skip property to noparameters-testCompile execution which is set to false only when there's a test that would match a filter.

[yrodiere]

I noticed a few typos, and added a few questions.

[yrodiere]

One more typo I noticed by chance. I also answered regarding the source filtering in maven-compiler-plugin.

[marko-bekhta]

ok. rebased to include the latest changes 😃

I've added a new execution for checkstyle to enforce the PackageName and updated the filters in the compiler plugin to use package rather than classname. I've left the SuppressionCommentFilter in there just in case.

I have also tried to remove the <compilerArgs combine.self="override" /> but with no luck - if it's not there, the sources are compiled with -parameters. (could it have something to do with https://github.com/hibernate/hibernate-search/blob/main/parents/integrationtest/pom.xml#L71-L72 ?)

Let's see if the build will pass on CI 🤞🏻

[yrodiere]

I've added a new execution for checkstyle to enforce the PackageName and updated the filters in the compiler plugin to use package rather than classname. I've left the SuppressionCommentFilter in there just in case.

👍

I have also tried to remove the <compilerArgs combine.self="override" /> but with no luck - if it's not there, the sources are compiled with -parameters. (could it have something to do with https://github.com/hibernate/hibernate-search/blob/main/parents/integrationtest/pom.xml#L71-L72 ?)

Right, that's probably the culprit. Changing that code you linked to configure the default-compile execution only should do the trick:

                        <execution>
                            <id>default-compile</id>
                            <configuration>
                                <compilerArgs combine.self="append">
                                    <compilerArg>-parameters</compilerArg>
                                </compilerArgs>
                            </configuration>
                        </execution>

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication