mapstruct / mapstruct

An annotation processor for generating type-safe bean mappers
https://mapstruct.org/
Other
7.04k stars 946 forks source link

Unmapped ghost collection properties #3425

Open foaw opened 10 months ago

foaw commented 10 months ago

Expected behavior

MapStruct doesn't report properties that aren't backed by a real field, and no warnings about this are issued. If the unmapped target policy is ERROR, the code gets compiled.

Actual behavior

When mapping from type A to type B, if type B has a getter method that returns collections, MapStruct treats it as a property to be populated. If the unmapped target policy is ERROR, an error is raised respectively.

A "collection" is used here as a collective term for really more than just j.u.Collection. Here are the types I tested this with and which were marked unmapped: Collection (List, Set, Queue, Deque), Map, Stream. What's worth nothing is that Iterable, Iterator, and Optional are fine.

Since the others are probably tested for their collection-ness with a single helper method, it also may be worthwhile marking Iterable, Iterator and Optional as collection types too.

Steps to reproduce the problem

public static class Foo {
    List<String> values;
    public List<String> getValues() { return values; }
    public void setValues(List<String> values) { this.values = values; }
}

public static class Bar {
    List<String> values;

    public List<String> getValues() { return this.values; }
    public void setValues(List<String> values) { this.values = values; }

    // This is the root of the problem; try replacing with any non-collection type.
    public Stream<String> getX() {
        throw new UnsupportedOperationException();
    }
}

@Mapper
public interface BarMapper {
    @Mapping(target = "values", source = "values")
    Bar toBar(Foo foo);
}

MapStruct Version

1.5.5.Final

filiphr commented 9 months ago

Can you try this with 1.6.0.Beta1? I think that we fixed it there (single getter collections are not in the error mapping), but I am not sure.

Keep in mind that you can use Mapper#collectionMappingStrategy with a value of CollectionMappingStrategy#TARGET_IMMUTABLE to make sure that the getters are not used as a setter.

MapStruct doesn't report properties that aren't backed by a real field, and no warnings about this are issued. If the unmapped target policy is ERROR, the code gets compiled.

MapStruct does do any assumption about the fields. It is using the Java Bean convention to decide if a method is exposing a property or not.

foaw commented 9 months ago

No luck, still picks them up with 1.6.0.Beta1 and no changes to the code. Tried it both with this made-up example above and in the actual project.

As for the fields, I really meant that there is no setter—my mistake. I also have somehow overlooked Mapper#collectionMappingStrategy, and I must admit that it does get the job done. However, when it's Stream's we're talking, shouldn't it always be TARGET_IMMUTABLE?

filiphr commented 9 months ago

However, when it's Stream's we're talking, shouldn't it always be TARGET_IMMUTABLE?

100% agree with you about this @foaw. We can't use a Stream as an alternative getter since it is an immutable. I've created #3462 to tackle this

foaw commented 9 months ago

Do you thing we could have an annotation for ignoring getters? Jackson has @JsonIgnore for this purpose. Let me know if I should open another issue for this, or just open one yourself and tag me.

filiphr commented 9 months ago

@foaw we used to have #1152 for exactly that. However, we decided against doing something like that with MapStruct. What you could do though is to implement your own AccessorNamingStrategy and check for an annotation in getMethodType and return MethodType.OTHER