Is it good practice to return ApiResponse from a Spring service layer?

Let me change your question a little bit, as it could be general instead of Spring based.

Is it good practice to return an API Response from a service layer?

There is going to be multiple comments here, nothing wrong about it since there are multiple ways of implementing a single solution.

However, from my perspective and taking good practices if you are doing a layer-based application there is an important rule to follow: each layer has its own responsibility and shouldn't carry the responsibility of another layer.

1. Service layers are where usually you perform the BL (Business Logic), validations, work with pure entities instead of DTOs and return pure entities too, is not responsible of returning API responses or transforming DTOs, that's why controller exists.

2. Controller layers are responsible of letting the clients know the DTOs, server responses (e.g operation results).

There's usually a confusion when handling service exceptions, it is really easy to question the following: If the service layer is not responsible of knowing the server responses, what happens with throwing exceptions in there?

I consider there's nothing wrong about it. Indeed, you need to have a way in order to throw exceptions when something happens. One thing is throwing exceptions and the other one throwing the server operation result.

Let's do a comparison table:

Is it good practice to return ApiResponse from a Spring service layer?

David Parnas proposed about modularization, and in particular the benefits of choosing your module boundaries so that the modules "hide" difficult design decisions or design decisions that are likely to change.

For design decisions that are NOT likely to change, separating the modules may not be so important.

Spring is plumbing - it's a way for the outside world to communicate with your voucher logic, but it certainly isn't the only way. If clean designs were free, we would certainly prefer a design that allows us to change the plumbing cheaply to a design that locks us in to a particular "vendor".

But if the decision to use spring is stable, and if the ApiResponse interface is stable, and so on, then there are probably other areas where you can invest effort to get better returns.

That said, if you did want to separate your voucher logic from the plumbing, it might go something like this.

First, we'd probably do a little refactoring to isolate the part of the code that is coupled to the ApiResponse. That might look like:

public ApiResponse<VoucherDto> claimVoucher(Long userId, Long templateId) {
   ...
        return returnValue(saved);

    } catch (DataIntegrityViolationException e) {
        return ApiResponse.error("You have already claimed this voucher");
    } catch (OptimisticLockException e) {
        return ApiResponse.error("Voucher claim failed, please try again (system busy)");
    }
}

// TODO: improve this name
ApiResponse<VoucherDto> returnValue(Voucher saved) {
   return ApiResponse.ok(toVoucherDto(saved));
}

With that idea in mind, we might then extract the returnValue function into a separate object

public ApiResponse<VoucherDto> claimVoucher(Long userId, Long templateId) {
   ...
        return this.someFactory.returnValue(saved);

    } catch (DataIntegrityViolationException e) {
        return ApiResponse.error("You have already claimed this voucher");
    } catch (OptimisticLockException e) {
        return ApiResponse.error("Voucher claim failed, please try again (system busy)");
    }
}

// TODO: improve this name
interface SomeFactory {
   // TODO: improve this name
   ApiResponse<VoucherDto> returnValue(Voucher saved);
}

And then comes the magic trick:

public T claimVoucher(Long userId, Long templateId) {
   ...
}

// TODO: improve this name
interface SomeFactory<T> {
   // TODO: improve this name
   T returnValue(Voucher saved);
}

So instead of having your voucher logic tightly coupled to ApiResponse, you have voucher logic that is tightly coupled to an interface, where one implementation of that interface is tightly coupled to the ApiResponse.

Complexity has increased (more parts to think about), but the responsibilities of the parts are more focused, and you have more freedom to assemble the parts in different ways.

Of course, you'd want to do something similar with the errors being returned

public T claimVoucher(Long userId, Long templateId) {
   ...
        return this.someFactory.returnValue(saved);

    } catch (DataIntegrityViolationException e) {
        return someFactory.returnValue(e);
    } catch (OptimisticLockException e) {
        return someFactory.returnValue(e);
    }
}

// TODO: improve this name
interface SomeFactory<T> {
   // TODO: improve this name
   T returnValue(Voucher saved);
   T returnValue(DataIntegrityViolationException e);
   T returnValue(OptimisticLockException e);
}

In the long term, you get the ability to change the details of the plumbing without impacting your voucher logic. In the short term, you maybe get better affordances for testing: the calculation of returned values can be tested independently of the branching logic, and the branching logic can be tested more flexibly, and without being tightly coupled to Spring.

Against this, you've split one thing into two -- there are now both more nodes in your code graph, and more edges between nodes; the cost of that may be small enough that you don't care, but it isn't free.

Surprise: it's trade offs all the way down.

To be short, the answer is clearly no, but I will use many approach to argument this answer:

• programmatic and pragmatic approach:
  You are using Spring ApiResponse, and what if you use @Controller and ResponseEntity? and what if you use @RestController instead?
  With " @Controller" annotation in spring you use a responseEntity, another class response, and you will have to recode your service. (Side note, I don't know what Spring module you use for ApiResponse, but that's not the main problem here).
  RestControllers doesn't use ApiResponse att all, and you will end up either unwrap the ApiResponse or your service, or modify all you service to fit your controller signature. Or even worse duplicate your service for every possible usage.

Basically, your controller and service layers are heavily coupled: You can only use the service for the designed controller, your service is supposed to be reusable, but you can't reuse your service because you have a responsibility problem, which make a transition to the next point

• SOLID Approach: Your service has one responsibility, SRP, it is business code. think that it could be called from a command line, another service, where all the HTTP information is useless. the controller manage the HTTP information, the service the only the business implementation, with no knowledge of the caller context.

• Hexagonal approach: which form my point of vue is an extreem approach to implements SOLID:
  Your service should only manage standard java, and not rely on Spring at all. If you are more pragmatic like me, you can use Spring injection (service/autowire) but that's all, no HTTP management, and even no database manipulation.
  So no code from Spring Web should be in the service Layer.

Whatever the code architecture or clean code habit you choose, using hardcoded class bound to a specific stack of your framework in the service will be a nightmare for maintenance and simply a useless constraint in development phase.

From your example, I see that you manage errors using HTTP code, the correct way to do it is to use java exception for errors, and let the normal return type "VoucherDto" without any wrapper. There are many mecanism in Spring to dispatch exception on the correct HTTP code.

From a helicopter view, since it is about a web application and HTTP protocol it's going to get used to send some data transfer object (DTO) from the server to the client, ApiResponse it is redundant: HTTP status code 200 OK, API success true OK. Hyper text transfer protocol (HTTP) includes information ApiResponse it's going to add and that increases network load/consumes bandwidth without benefits or at least without obvious benefits. Either HTTP is going to be avoided and then ApiResponse is HTTP minimum viable replacement, either HTTP is going to be used and ApiResponse should be avoided.

A controller and a service are separate things for a reason: They focus on different things.

The service should focus on the business logic and should not be concerned with HTTP status codes, JSON formats, etc... It should be possible to use the service from a controller, but also be driven from a CLI instead of an HTTP endpoint without change.

A controller should focus on the HTTP stuff: status codes, JSON response format, request validation, etc...

Taking your example, I would rewrite the service method like this:

@Transactional
public Voucher claimVoucher(Long userId, Long templateId) {
    VoucherTemplate template = templateRepository.findById(templateId)
            .orElseThrow(() -> new VoucherTemplateNotFoundException(templateId));

    LocalDateTime now = TimeUtils.now();

    if (!template.isActive() || template.isArchived())
        throw new VoucherTemplateInActiveException(templateId);

    if (template.getValidFrom().isAfter(now))
        throw new VoucherTemplateNotActiveYetException(templateId);

    if (template.getValidTo().isBefore(now))
        throw new VoucherTemplateExpiredException(templateId);

    if (template.getClaimedCount() >= template.getTotalQuantity())
        throw new VoucherTemplateOutOfStockException(templateId);

    if (voucherRepository.existsByUserIdAndTemplate_Id(userId, templateId))
        throw new VoucherTemplateAlreadyClaimedException(templateId, userId);

    try {
        template.setClaimedCount(template.getClaimedCount() + 1);
        templateRepository.saveAndFlush(template);

        Voucher voucher = Voucher.builder()
                .template(template)
                .userId(userId)
                .active(true)
                .used(false)
                .claimedAt(now)
                .validFrom(template.getValidFrom())
                .validTo(template.getValidTo())
                .build();

        Voucher saved = voucherRepository.save(voucher);
        return saved;

    } catch (DataIntegrityViolationException e) {
        throw new VoucherTemplateAlreadyClaimedException(templateId, userId);
    } catch (OptimisticLockException e) {
        throw new VoucherTemplateException(e);
    }
}

Some things to note:

• The method returns the Voucher object, not a DTO.
• The method throws dedicated business exceptions when there is a problem.

If you change the service like that, then the controller becomes:

@PostMapping("/claim/{templateId}")
@PreAuthorize("hasRole('USER')")
public VoucherDto claimVoucher(
        @AuthenticationPrincipal CustomUserDetails user,
        @PathVariable("templateId") Long templateId
) {
    return VoucherDto.of(voucherService.claimVoucher(user.getId(), templateId));
}

For this to work, you need to create a VoucherDto.of(Voucher) method that does the conversion from the Voucher domain object to the VoucherDto web layer object. Personally, I would rename VoucherDto to VoucherResponse probably, but that is entirely optional.

For the error cases, I use the error-handling-spring-boot-starter (Note: I am the author of the library) which will automatically translate the exception into a nice JSON error.

For example, take this exception:

@ResponseStatus(HttpStatus.BAD_REQUEST)
public class VoucherTemplateExpiredException extends RuntimeException {

  public VoucherTemplateExpiredException(String templateId) {
    super("The voucher template %s is expired.".formatted(templateId));
  }
}

This would return this JSON (by default, you can customize this if needed):

{
  "code": "VOUCHER_TEMPLATE_EXPIRED",
  "message": "The voucher template 123 is expired."
}

With the HTTP status code of 400.

If you want to expose the expiry date as a field in the JSON response, then you can create a method or a field in the exception and annotate it with @ResponseErrorProperty

You can read more about how I do this layering in my blog entry How I write production-ready Spring Boot applications.