Add @Value.Builder record builder support to ImmutablesBuilderProvider#4006
Open
rlei wants to merge 3 commits intomapstruct:mainfrom
Open
Add @Value.Builder record builder support to ImmutablesBuilderProvider#4006rlei wants to merge 3 commits intomapstruct:mainfrom
rlei wants to merge 3 commits intomapstruct:mainfrom
Conversation
Author
|
Hi @filiphr could you please take a look at this one? With minor updates, this will also support the another |
When a Java record is annotated with Immutables' @Value.Builder
(supported since Immutables 2.11.0), the annotation processor generates
a companion {RecordName}Builder class (e.g. GolfPlayerBuilder for record
GolfPlayer) rather than the traditional Immutable{TypeName} implementation
used for interfaces and abstract classes.
The existing ImmutablesBuilderProvider only looked for @Value.Immutable
and the ImmutableXxx naming convention, so it could not detect builders
for @Value.Builder-annotated records.
This commit adds:
- findBuilderInfoForImmutablesRecord(): detects records annotated with
@Value.Builder and looks up the companion {RecordName}Builder type
- asImmutableRecordBuilderElement(): constructs the qualified name of
the generated builder following the {RecordName}Builder convention
- findBuilderInfoFromCompanionBuilder(): constructs BuilderInfo directly
from the companion class using its public no-arg constructor as the
builder creation method and findBuildMethods() to locate build().
Immutables always generates a public no-arg constructor (not a static
factory) for @Value.Builder on records, mirroring how
DefaultBuilderProvider handles inner *Builder classes.
- immutablesRecordBuilderTest integration test that exercises end-to-end
mapping through the Immutables-generated record builder using
Immutables 2.11.0, registered in MavenIntegrationTest and restricted
to JDK compiler on Java 16+. Tests include a @nullable field to show
that the builder's mandatory-field enforcement fires correctly when
NullValueCheckStrategy.ALWAYS causes MapStruct to skip setting a null
mandatory field, whereas falling back to the constructor would silently
accept null.
ElementKind.RECORD was added in Java 16; the implementation uses
typeElement.getKind().name() string comparison to remain compilable
on Java releases that pre-date records.
rlei
force-pushed
the
feature/immutables-record-builder-support
branch
from
00c2f52 to
8037de2
Compare
Adds unit tests for the new ImmutablesBuilderProvider record-builder support, covering the code paths not exercised by integration tests: - Value.java: minimal stub of org.immutables.value.Value (with both @immutable and @builder inner annotations) so that the test compilation activates ImmutablesBuilderProvider as the default builder provider without requiring the Immutables jar - GolfPlayer: a @Value.Builder-annotated record with a nullable Integer handicap field and mandatory name/age fields - GolfPlayerBuilder: hand-crafted stand-in for the Immutables-generated companion builder, using an initBits bitmask to enforce mandatory fields at build() time - ImmutablesRecordBuilderTest: three @ProcessorTest(Compiler.JDK) tests covering the happy path, nullable field handling, and mandatory-field enforcement via NullValueCheckStrategy.ALWAYS
…rProvider Covers the findBuilderInfoForImmutables() code path where a @Value.Immutable annotated interface is mapped through the ImmutableXxx generated implementation and its nested Builder, improving instruction coverage from 84% to 88%.
Author
|
It seems the integration tests in the first commit was not considered in the coverage report. @filiphr I've added unit tests to cover ImmutablesBuilderProviders, could you please check again? |
filiphr
requested changes
Mar 21, 2026
Member
filiphr
left a comment
filiphr
left a comment
There was a problem hiding this comment.
Thanks for adding the tests in the processor module @rlei. I was going to merge it without it as well, but it's better to have them closer. However, I think that we should not add the @Value like you did. I've left some comments inline. If we address those then I'll go ahead and merge this.
| * @param typeElement the record type element | ||
| * @return the {@link TypeElement} for the generated builder, or {@code null} if not yet available | ||
| */ | ||
| protected TypeElement asImmutableRecordBuilderElement(TypeElement typeElement) { |
Member
There was a problem hiding this comment.
The only difference between this and asImmutableElement is the prefix vs suffix. To reduce the duplication we could extract a shared method like:
private TypeElement findRelatedTypeElement(TypeElement typeElement, String prefix, String suffix) {
Element enclosingElement = typeElement.getEnclosingElement();
StringBuilder qualifiedName = new StringBuilder(typeElement.getQualifiedName().length() + 17);
if (enclosingElement.getKind() == ElementKind.PACKAGE) {
qualifiedName.append(((PackageElement) enclosingElement).getQualifiedName().toString());
} else {
qualifiedName.append(((TypeElement) enclosingElement).getQualifiedName().toString());
}
if (qualifiedName.length() > 0) {
qualifiedName.append(".");
}
qualifiedName.append(prefix).append(typeElement.getSimpleName()).append(suffix);
return elementUtils.getTypeElement(qualifiedName);
}Then:
asImmutableElement->findRelatedTypeElement(typeElement, "Immutable", "")asImmutableRecordBuilderElement->findRelatedTypeElement(typeElement, "", "Builder").
Author
There was a problem hiding this comment.
Thank you @filiphr for the review! Will update the PR shortly.
Member
There was a problem hiding this comment.
If we are adding tests to the processor module then let's do this properly. We should not using other dependency packages. Instead we could add the immutables as a dependency or we could do something similar like we are doing in org/mapstruct/ap/test/bugs/_1801. I think adding it as a dependency is OK.
| }) | ||
| class ImmutablesRecordBuilderTest { | ||
|
|
||
| @ProcessorTest(Compiler.JDK) |
Member
There was a problem hiding this comment.
Why only Compiler.JDK? Are there some problems with Eclipse?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ImmutablesBuilderProvidersupports@Value.Immutableon interfaces and abstract classes, where Immutables generates anImmutableXxximplementation containing a nestedBuilder. However, since Immutables 2.11.0, Java records can be annotated with@Value.Builder, which causes Immutables to generate a top-level companion{RecordName}Builderclass instead. The existing provider did not handle this case, causing MapStruct to fall back to calling the record constructor directly — bypassing the builder's mandatory-field enforcement.Solution
findBuilderInfoForImmutablesRecord(): detects records annotated with@Value.Builderand looks up the companion{RecordName}Buildertype.asImmutableRecordBuilderElement(): constructs the companion builder's qualified name following the{RecordName}Builderconvention.findBuilderInfoFromCompanionBuilder(): constructsBuilderInfodirectly using the companion class's public no-arg constructor (which Immutables always generates for@Value.Builderon records) andfindBuildMethods()to locatebuild().immutablesRecordBuilderTestintegration test using Immutables 2.11.0, restricted to JDK compiler on Java 16+. Includes a@Nullablefield andNullValueCheckStrategy.ALWAYSto demonstrate that the builder's mandatory-field check fires when MapStruct skips a null setter — behaviour that would be silently lost if MapStruct fell back to the record constructor.Notes
ElementKind.RECORDwas added in Java 16; the implementation usestypeElement.getKind().name()string comparison to remain compilable on older Java releases.ImmutablesBuilderProvideris automatically activated whenorg.immutables.value.Value.Immutableis on the classpath (seeAnnotationProcessorContext);@Value.Builderis part of the same artifact so no additional activation is needed.