Spring REST Endpoint for Saving a User Inquiry

Question

Here's a piece of code from codebase I maintain/contribute to. It is a Spring REST controller, which exposes an endpoint to create user question. The question consists of title, description and reference to the user who asked it.

It feels it's a bit of problematic from the design standpoint. That said, the person who originally wrote this controller is more experienced than I am so I want to make sure. To me, the method below seems bloated and violating the Single Responsibility Principle (SRP)

@RequiredArgsConstructor
@Validated
@RestController
@RequestMapping("/api/v1/user/questions")
public class UserQuestionRestController {

    private final QuestionService questionService;
    private final TagService tagService;
    private final QuestionResponseDtoService questionResponseDtoService;

    @PostMapping
    public ResponseEntity<Void> create(@RequestBody @Valid QuestionRequestDto dto,
                                       @NotNull Authentication authentication) {
        Question question = QuestionMapper.toEntity(dto);
        Account currentUser = (Account) authentication.getPrincipal();
        question.setOwner(currentUser);
        List<Tag> tags = new ArrayList<>();
        if (dto.tagIds() != null) {
            tags = tagService.getByIds(dto.tagIds());
        }
        question.setTags(new HashSet<>(tags));
        questionService.create(question);
        return ResponseEntity.status(HttpStatus.CREATED).build();
    }

public final class QuestionMapper {

    private QuestionMapper() {
    }

    public static Question toEntity(QuestionRequestDto dto) {
        Question question = new Question();
        question.setTitle(dto.title());
        question.setDescription(dto.description());
        return question;
    }
}

First, the controller does so much more that simply receiving a request and sending a response. Second, there's an attempt to separate concerns with the QuestionMapper static method toEntity() which doesn't do the "DTO → entity" mapping in its entirety anyway. The owner and tags fields are set in the controller itself. That is, it's what seems to me a SRP violation happening twice

If I am right, and there's a need to move all that logic from the controller somewhere else, what that "somewhere else" should be? I could make QuestionMapper a @Component, add and @Autowire a service field, and then set all the fields in the toEntity() method (I also doubt it should be static in this case). Eventually, I ditched the QuestionMapper altogether and do the conversion with the help of a service instance.

@Validated
@RequiredArgsConstructor
@RestController
@RequestMapping("/api/v1/user/questions")
public class UserQuestionRestController {
    private final QuestionService service;
    @PostMapping
    public ResponseEntity<Void> create(@RequestBody @Valid QuestionRequestDto dto,
                                              @NotNull Authentication authentication) {
        Question question = service.mapFromDto(dto, authentication);
        service.create(question);
        return ResponseEntity.created(/* URL */).build();
    }
}

Would you agree that this is the better way to do it?

Here is the Question entity for completeness.

@AllArgsConstructor
@NoArgsConstructor
@Getter
@Setter
@Entity
@EntityListeners(AuditingEntityListener.class)
@Table(name = "questions")
public class Question {

    @Setter(AccessLevel.NONE)
    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "id", unique = true, nullable = false, updatable = false)
    private Long id;

    @Setter(AccessLevel.NONE)
    @NotNull
    @CreatedDate
    @Column(name = "created_date", nullable = false)
    private LocalDateTime createdDate;

    @Setter(AccessLevel.NONE)
    @NotNull
    @LastModifiedDate
    @Column(name = "modified_date", nullable = false)
    private LocalDateTime modifiedDate;

    @NotBlank
    @Column(name = "title", nullable = false)
    private String title;

    @NotBlank
    @Column(name = "description", nullable = false, columnDefinition = "text")
    private String description;

    @NotNull
    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "account_id", nullable = false, updatable = false)
    private Account owner;

    @NotNull
    @ManyToMany
    @JoinTable(name = "questions_tags",
            joinColumns = @JoinColumn(name = "question_id", nullable = false),
            inverseJoinColumns = @JoinColumn(name = "tag_id", nullable = false),
            uniqueConstraints = @UniqueConstraint(columnNames = {"question_id", "tag_id"}))
    private Set<Tag> tags = new HashSet<>();

Answer 1

Regarding the mapper

public final class QuestionMapper {
    public static Question toEntity(QuestionRequestDto dto) {
        ...
    }
}

The way the mapper is implemented is an invitation to SRP violations. You have a mapper for a type, and the mapper is responsible for converting that type to whatever other object type there is a need for. What happens if you need to create a QuestionSummary object? By the way the mapper is named, the toQuestionSummary() method would go to QuestionMapper. With your design, the possible set of responsibilities for a given mapper are not restricted. I have seen the worst possible results for this approach and they were not pretty.

My preferred choice is to "reverse" the mapper. A QuestionMapper would be responsible for creating Question objects from whatever type there is. While this doesn't guarantee 1-to-1 mapping, it has worked really well for me and the mappers that have multiple mapping methods are a rarity.

My chosen name for all mapping methods is from. This ensures that the naming is always consistent between mappers and that naming conflicts are a sign of a SRP violation or other code smell. And there is no need to spend time figuring out the correct name for a mapping method (e.g. should your method be toDto, toEntity or toQuestionRequest?) The name of the input parameter for the from-method is always source. The name for the target object is always... target. These last two are a matter of taste, you could use "input" and "output" or whatever, as long as the choice is consistent throughout the codebase.

The mappers are components that get injected. A mapper may depend on another mapper, in case the mapped type is a complex object or contains common mappings. The dependencies are again injected. This allows efficient unit testing as the mappers can be easily mocked.

Your mapper would thus become:

@Component
public class QuestionMapper {
    public Question from(final QuestionRequestDto source) {
        final Question target = new Question();
        ...
        return target;
    }
}