Conversation
…eckStrategy.ALWAYS Signed-off-by: Marvin Froeder <velo.br@gmail.com>
|
@ijioio your comments here and on #4123 were deleted, quoting from the notification emails:
Agreed on The three-state patch behaviour itself is |
|
@velo Yes, I've deleted my previous comment here cause I've confused I'm still not sure whether the public class Source {
private Optional<String> foo;
public Optional<String> getFoo() { return foo; }
public void setFoo(Optional<String> foo) { this.foo = foo; }
}
public class Target {
private String foo;
public String getFoo() { return foo; }
public void setFoo(String foo) { this.foo = foo; }
}
@Mapper
public interface OptionalMapper {
public static final OptionalMapper INSTANCE = Mappers.getMapper(OptionalMapper.class);
public default <T> T extract(Optional<T> optional) {
return optional.orElse(null);
}
}
@Mapper(uses = OptionalMapper.class, nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE)
public interface PatchMapper {
public static final PatchMapper INSTANCE = Mappers.getMapper(PatchMapper.class);
public void patch(@MappingTarget Target target, Source source);
}So, |
2 participants
Fixes #4123
Problem
For an
Optionalsource property MapStruct generates a bare presence check:If the
Optional-typed property is itselfnull, that throws aNullPointerException. AnullOptionalfield is easy to reach in practice: Lombok's@Builderleaves an unsetOptionalcomponent asnull, and Jackson leaves itnullwhen the JSON property is absent.The release notes state the missing
nullcheck is intentional, and this PR keeps that default. The actual defect is thatnullValueCheckStrategy = NullValueCheckStrategy.ALWAYShas no effect onOptionalsource properties: the generated method body is identical with and without it, which the committed fixtureOptionalNullCheckAlwaysMapperImpl.javashows.ALWAYSis documented as "always include a null check when source is non primitive", so a source that can benullis exactly what it is for.The reason is that an
Optionalsource already forcesincludeSourceNullCheckon through the type conversion, so theALWAYSbranch insetterWrapperNeedsSourceNullChecknever changes the outcome, and the template renders the check as.isPresent().Fix
PropertyMappingnow builds an explicit presence check for anOptionalsource property whenNullValueCheckStrategy.ALWAYSis configured, composing the existingNullPresenceCheckandOptionalPresenceCheck:Scope
nullValueCheckStrategy = ALWAYS. The defaultON_IMPLICIT_CONVERSIONis unchanged, so no existing user sees a different mapper.@NonNullsource still skips the guard, consistent with JSpecify taking precedence overNullValueCheckStrategyelsewhere.Optionalsource parameter (Target map(Optional<Source> src)) is still checked with a bareisPresent(), and Optional<Map> / Optional<Collection> source property generates unguarded Optional#get, throwing NoSuchElementException when empty #4111 (missing presence check forOptional<Map>/Optional<Collection>) is unrelated.Tests
Extended
org.mapstruct.ap.test.optional.nullcheckalways:optionalToOptionalWhenNull/optionalToNonOptionalWhenNull— anullOptionalproperty no longer throws and the target is left untouched.OptionalNullCheckAlwaysMapperImplfixture to the guarded form.OptionalDefaultNullCheckMapperplus fixture, pinning the unchanged default-strategy output.The existing empty/present cases and the other
optionalfixtures (default strategy) are unchanged.mvn installon JDK 21:MapStruct Processormodule green, 3649 tests, 0 failures. Theintegrationtestmodule was not run locally (it spawns nested Maven builds that need network access).Documentation updated in
chapter-3(Optional section) andchapter-10(NullValueCheckStrategy).