Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MapStruct Mapper Fails to Inject Dependencies When Lombok and Spring Are Used Without Mocks in Tests #28592

Open
YassineOularbi opened this issue Jan 30, 2025 · 8 comments

Comments

@YassineOularbi
Copy link

Overview of the issue

When using MapStruct with Lombok in JHipster-generated mappers, tests fail with NullPointerException due to missing dependency injection (e.g., UserActivityMapper not being injected). This occurs in non-mocked tests where mappers depend on other mappers.

Error Example:
[ERROR] ApiErrorMapperTest.shouldConvertToDtoAndBack:21 » NullPointer
Cannot invoke "UserActivityMapper.toDto(...)" because "this.userActivityMapper" is null

Motivation for or Use Case

This bug prevents testing mapper conversions without using Mockito mocks (@Mock/@InjectMocks). It affects JHipster projects relying on Spring-managed dependency injection for MapStruct.

Reproduce the error
  1. Generate a JHipster entity with DTO using MapStruct.
  2. Create a mapper that depends on another mapper (e.g., ApiErrorMapper uses UserActivityMapper).
  3. Write a test without mocks:
    class ApiErrorMapperTest {
        private ApiErrorMapper apiErrorMapper = new ApiErrorMapperImpl();
        
        @Test
        void shouldConvertToDtoAndBack() {
            ApiErrorEntity entity = new ApiErrorEntity();
            apiErrorMapper.toDto(entity); // Throws NPE
        }
    }
@atomfrede
Copy link
Member

I think creating a mapper via new and using the generated MapperImpl is at least discouraged. You should create mappers via Mappers.getInstance but I suppose that's not working with spring injected beans into mappers.

@OmarHawk
Copy link
Contributor

The default JHipster Mapper Tests are also generated like this.

class <%= entityClass %>MapperTest {
private <%= entityClass %>Mapper <%= entityInstance %>Mapper;
@BeforeEach
void setUp() {
<%= entityInstance %>Mapper = new <%= entityClass %>MapperImpl();
}
@Test
void shouldConvertToDtoAndBack() {
var expected = get<%= persistClass %>Sample1();
var actual = <%= entityInstance %>Mapper.toEntity(<%= entityInstance %>Mapper.toDto(expected));
assert<%= persistClass %>AllPropertiesEquals(expected, actual);
}
}

Since we also adjusted our mappers to make use of this as we need a bit more complex objects:

@Mapper(componentModel = "spring", uses = { SomeOtherEntityMapper.class })
public interface EntityAMapper extends EntityMapper<EntityADTO, EntityA>

we had to also adjust the corresponding tests manually ;-)

e.g.:

@IntegrationTest
@AutoConfigureMockMvc
class EntityAMapperTest {

    @Autowired
    private EntityAMapper entityAMapper;

@atomfrede
Copy link
Member

@OmarHawk Interesting (the generated tests use the Impl directly. It will only work for mappers that do not user other mappers. I try to use mappers without injected beans (using context or additional parameters) to have mappers that work without any injection framework.

@OmarHawk
Copy link
Contributor

OmarHawk commented Jan 31, 2025

Ok, well, for us it is more important to not duplicate mapping code and having a consistent mapping for sub-entities, so injection of an already well-defined mapper for another Entity instead of having to write own code for that is beneficial to us. ;-)

@atomfrede
Copy link
Member

But you would not need spring injection for that (off topic discussion) when all mappers are not using any injection model

@YassineOularbi
Copy link
Author

YassineOularbi commented Jan 31, 2025

I fixed the problem by injecting the mapper directly instead of the Impl using @Injectmocks and injecting required beans with @mock.

@ExtendWith(MockitoExtension.class)
class UserSessionMapperTest {

    @InjectMocks
    private UserSessionMapper userSessionMapper;

    @Mock
    private GeoLocationMapper geoLocationMapper;

    @Mock
    private UserMapper userMapper;

    @BeforeEach
    void setUp() {
    }

    @Test
    void shouldConvertToDtoAndBack() {
        var expected = getUserSessionEntitySample1();
        var actual = userSessionMapper.toEntity(userSessionMapper.toDto(expected));
        assertUserSessionEntityAllPropertiesEquals(expected, actual);
    }
}

I don't know if its best practice or not

@mshima
Copy link
Member

mshima commented Feb 5, 2025

@OmarHawk should we do anything related to this issue?

@OmarHawk
Copy link
Contributor

OmarHawk commented Feb 7, 2025

Well, at least for avoiding code duplications for the MapStructExpression fields, this would be helpful. At the moment, that expression is duplicated into every mapper that accesses such a field, e.g. (manually typed here, hope there's no syntax error)

entity CentralConfigEntity {
  name String
  value String
  configType String
  @hidden
  @MapstructExpression("java(s.getConfigType() + Character.toString('|') + s.getName())")
  details String
}

entity NeedsSomeConfigA {
  name String
}

entity NeedsSomeConfigB {
  name String
}

entity NeedsSomeConfigC {
  name String
}

relationship OneToMany {
  CentralConfigEntity{ea} to NeedsSomeConfigA{confA(details)}
  CentralConfigEntity{eb} to NeedsSomeConfigB{confB(details)}
  CentralConfigEntity{ec} to NeedsSomeConfigC{confC(details)}
}

produces in Mapper class of NeedsSomeConfigA, NeedsSomeConfigB, NeedsSomeConfigC the very same expression String as it is already there in CentralConfigEntity.

Also that generated mapper method in that very same Mapper class strips out every field from the other entity that is not needed from the frontend (usually just primary key & these expression fields are kept) and therefore wouldn't even consider the third level of nesting of such entities.
From a resource perspective, this is actually good in the default case, as useless data transfer is avoided.

But in our case we do need in some places deeply nested objects as a complete thing in our frontend, so we had to adjust the mappers as mentioned above and also implicitly got rid of these duplicated expressions there.
And since we didn't want to duplicate the mapping code, we just used that uses parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants