Clean code. It is okay to use repository inside Entity-DTO mapper? | Together Java | Page 1

icy olive Jun 17, 2025, 4:42 AM

#

@Component
@RequiredArgsConstructor
public class MaterialUnitMapper {

  private final StockContainerRepository stockContainerRepository;

  public MaterialUnit toEntity(MaterialUnitDTO dto) {
        MaterialUnit unit = new MaterialUnit();

        unit.setName(dto.getName());

        StockContainer stockFromDb = stockContainerRepository.findById(
            dto.getStockContainerId()
          );

        unit.setStockContainer(stockFromDb);

        return unit;
    }
}

Is it okay to hold repository inside mapper? I wonder wether repository should be accessable only from service layer.

carmine yewBOT Jun 17, 2025, 4:42 AM

#

<@&987246883653156906> please have a look, thanks.

#

Clean code. It is okay to use repository inside Entity-DTO mapper?

#

Changed the title to Clean code. It is okay to use repository inside Entity-DTO mapper?.

primal trout Jun 17, 2025, 5:08 AM

#

icy olive ```java @Component @RequiredArgsConstructor public class MaterialUnitMapper { ...

id say no. a mapper shouldnt do complex logic. but the question is if u have an alternative

icy olive Jun 17, 2025, 5:13 AM

#

primal trout id say no. a mapper shouldnt do complex logic. but the question is if u have an ...

I was thinking about leaving the basic variables in the mapper and finishing the mapping logic in stockContainerService

primal trout Jun 17, 2025, 5:16 AM

#

dunno if thats better. that trips people as well, seeing mapping done outside of the mapper

icy olive Jun 17, 2025, 5:18 AM

#

@Component
public class MaterialUnitMapper {

  public MaterialUnit toEntity(MaterialUnitDTO dto) {

        return new MaterialUnit(dto.getName());
    }
}

@Service
@RequiredArgsConstructor
public class MaterialUnitService {

  private final MaterialUnitMapper mapper;
  private final MaterialUnitRepository unitRepository;
  private final StockContainerRepository containerRepository;
  
  public MaterialUnit toEntity(MaterialUnitDTO dto) {
    MaterialUnit unit = mapper.toEntity(dto);

    StockContainer stockFromDb = containerRepository.findById(
            dto.getStockContainerId()
          );

    unit.setStockContainer(stockFromDb);
  }
}

icy olive Jun 17, 2025, 5:19 AM

#

primal trout dunno if thats better. that trips people as well, seeing mapping done outside of...

That's why I'm asking because it's a bit confusing in both cases. Mapper should do all the work instead of finishing it in service

primal trout Jun 17, 2025, 5:19 AM

#

thats worse bc now ur mapper spits out unfinished data

#

if u want to go that route u should introduce another class

#

so that the service doesnt just use a setter but maps to another class instead

#

preventing people from accidentally using the result from the mapper directly

#

but imo the main problem here is design-wise to question why u even need a mapper to decorate sth with data from a repo. the mapper is supposed to map from a foreign data model to the data model u want to use in ur application (or the other way around). so why do u need to add sth from the DB to it.

icy olive Jun 17, 2025, 5:21 AM

#

primal trout if u want to go that route u should introduce another class

Do you have other alternative maybe? To me this seems to be an even worse case than calling repository in the mapper because it creates boilerplatecode and can be confusing. Why map twice in installments?

primal trout Jun 17, 2025, 5:22 AM

#

why is this DB thing even considered part of the dto

#

why doesnt the service that needs to work with the DTO get hands on the thing from the DB and it then wont be part of the DTO

icy olive Jun 17, 2025, 5:22 AM

#

DTO hold unit name and stockContainerId. Entity have name and StockContainer stockContainer

#

public class MaterialUnit {

    @Id
    @Nullable
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;
    private String name;
    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "stock_container_id")
    @JsonIgnore
    @ToString.Exclude
    @EqualsAndHashCode.Exclude
    private StockContainer stockContainer;
}

public class MaterialUnitDTO {

    private Long id;
    @NotBlank
    private String name;
    private Long stockContainerId;
}

icy olive Jun 17, 2025, 5:28 AM

#

primal trout why doesnt the service that needs to work with the DTO get hands on the thing fr...

Wait okay i understand now.

#

But what if i want create new MaterialUnit? I post request body {"name": "Unit1", "stockContainerId": "7"}

@RestController
@RequiredArgsConstructor
public class MaterialUnitController() {
  
  private final MaterialUnitService service;

  public void create(@RequestBody MaterialUnitDTO dto) {
    service.create(dto);
  }
}

@Service
@RequiredArgsConstructor
public class MaterialUnitService {

  private final MaterialUnitMapper mapper;
  private final MaterialUnitRepository unitRepository;
  
  public void create(MaterialUnitDTO dto) {
    MaterialUnit unit = mapper.toEntity(dto);

    StockContainer stockFromDb = containerRepository.findById(
            dto.getStockContainerId()
          );

    unit.setStockContainer(stockFromDb);

    repository.save(unit);
  }
}

#

In both cases I have to somehow get the container from the database to check if container with id=7 exist

primal trout Jun 17, 2025, 5:42 AM

#

to me what ur doing isnt "mapping" anymore. the point of a mapper is to create a ACL. not to add business logic to ur data.
when u want to transfer a Person class coming from another service/api into the Person class u use internally but that one has int amountOfFriends, then thats not the task of a mapper anymore

#

cause u cant answer that without complex business logic

#

its a design problem. one that requires stepping back

#

one that probably needs 3 distinct DTO classes instead

#

i cant come up with good names now, but essentially: PersonFromOtherService, PersonForOurUse, PersonWithFriendsData

#

and the mapper would take care of going PersonFromOtherService -> PersonForOurUse to create a ACL

#

and PersonWithFriendsData is done by a service, utilizing the DB and whatnot

#

its not a "mapping" anymore, its complex business logic

#

but if u cant find a proper setup where doing sth like that makes sense and id have to pick between the two evils u offered, id probably put the DB into the mapper

icy olive Jun 17, 2025, 5:45 AM

#

The problem became a little clearer

#

So in my case if i understand correctly. If i wanna create new MaterialUnit, i should create something like second DTO (MaterialUnitForSaveDTO) with second mapper?

icy olive Jun 17, 2025, 6:00 AM

#

primal trout but if u cant find a proper setup where doing sth like that makes sense and id h...

I saw code of experienced guy who has similar problem with mine but he use UserService instead UserRepository

@Component
@RequiredArgsConstructor
public class CarRentMapper {
    private final UserMapper userMapper;
    private final UserService userService;

    CarRentDto toDto(CarRent carRent) {
        return new CarRentDto(carRent.getId(),
                userMapper.toDto(carRent.getUser()),
                carRent.getRentDate(),
                carRent.getEndDate(),
                carRent.getPrice()
        );
    }

    CarRent toEntity(CarRentDto carRentDto) {
        User user = userService.findUser(carRentDto.userId())
            .orElseThrow(() -> new IllegalArgumentException("User with ID "                           + carRentDto.userId() + " not found"));
        return new CarRent (
                user,
                carRentDto.getRentDate(),
                carRentDto.getEndDate(),
                carRentDto.getPrice(),
                carRentDto.getStartDistance(),
                carRentDto.getEndDistance()
        );
    }
}

But still business logic is inside mapper

icy olive Jun 17, 2025, 6:25 AM

#

@primal trout Sorry for ping but I think I found a better solution sending StockContainer to function in mapper, can you take a look?

@Service
@RequiredArgsConstructor
class MaterialUnitService {

    private final MaterialUnitRepository repository;
    private final MaterialUnitMapper mapper;
    private final StockContainerService stockContainerService;

    @Override
    public MaterialUnitDTO create(MaterialUnitDTO dto) {

        StockContainer stockContainer = null;
        if (dto.getStockContainerId() != null) {
            stockContainer = stockContainerService.findById(dto.getStockContainerId());
        }

        MaterialUnit entity = mapper.toEntity(dto, stockContainer);
        MaterialUnit savedEntity = repository.save(entity);
        return mapper.toDTO(savedEntity);
    }
}

#

@Component
@RequiredArgsConstructor
public class MaterialUnitMapper {

    private final StockContainerService stockContainerService;

    public MaterialUnit toEntity(MaterialUnitDTO dto, StockContainer stockContainer) {
        MaterialUnit unit = new MaterialUnit();

        unit.setId(dto.getId());
        unit.setExternalId(dto.getExternalId());
        unit.setName(dto.getName());
        unit.setLocation(dto.getLocation());
        unit.setStockContainer(stockContainer);

        return unit;
    }
}

primal trout Jun 17, 2025, 6:27 AM

#

the general approach is better id say, yes.
the problem im having is that its still the same type

#

public MaterialUnitDTO create(MaterialUnitDTO dto) {

#

ur DTO now has a field that is sometimes null, sometimes not null

#

this is dangerous

#

ull run into bugs down the line with that design

#

it would be better if u can separate these two from each other

#

for example by having 2 distinct classes

#

one that never has a container and one that always has one

#

that said, there is a bug in this logic currently:

#

StockContainer stockContainer = null;
if (dto.getStockContainerId() != null) {
  stockContainer = stockContainerService.findById(dto.getStockContainerId());
}

MaterialUnit entity = mapper.toEntity(dto, stockContainer);

#

stockContainer remains null if the dto already had a container

#

in which case ur mapper crashes

#

u probably intended to do it like this instead:

#

StockContainer stockContainer = dto.getStockContainerId();
if (stockContainer != null) {
  stockContainer = stockContainerService.findById(dto.getStockContainerId());
}

MaterialUnit entity = mapper.toEntity(dto, stockContainer);

#

changes: https://i.imgur.com/5HErMK7.png

Imgur

#

oh, and it probably should be == null, not != null

#

but yeah, details. the approach itself looks better

icy olive Jun 17, 2025, 6:32 AM

#

primal trout one that never has a container and one that always has one

Thank you, you're right, there is a bug here and it's so basic it's embarrassing

primal trout Jun 17, 2025, 6:32 AM

#

dont worry. these bugs happen quickly and u should rather blame the design than ur brain

#

objects with data that is sometimes null, sometimes not, quickly lead to logic bugs

icy olive Jun 17, 2025, 6:34 AM

#

Thanks for help, i think that's all. I'll leave thread if someone wanna read and have similar problem

worldly tendon Jun 17, 2025, 1:07 PM

#

with JPA having a repository wired is usually not necessary because JPA can make the repo call itself if you try getting a field which is mapped as a relationship

#

or, in the case of 1<->1 relationships, you can preload them in advance

#Clean code. It is okay to use repository inside Entity-DTO mapper?