A typical controller method to process a request body currently looks something like this:
class MyController {
@PostMapping("/comments")
HttpEntity<?> postComment(@Valid @RequestBody CommentDto payload, Errors errors) { … }
}
The method separates the concept of a validated request body into the actual payload as well as potentially occurred binding and validation errors as a separate object. However, there is an implicit requirement of the Errors instance having to follow the @RequestBody annotated object immediately as there could potentially multiple ones involved in a single request.
It would be cool if there was a single type that could be declared on a handler method that captures the individual aspects involved here:
class MyController {
@PostMapping("/comments")
HttpEntity<?> postComment(Payload<CommentDto> payload) { … }
}
That type would expose both the mapped target object (CommentDto) as well as the Errors to the controller method. Ideally, that framework provided abstraction could then in turn be transformable into a user defined type, that could the provide specialized API to process the request. E.g. in a project I recently used a monadic type to define the processing in a pipeline-like way:
return MappedPayloads.of(payload, errors)
.notFoundIf(…) // always turn into 404 if predicate matches
.map((it, err) -> representations.from(it, err)) // *always* invoke translation into domain object potentially producing additional errors
.mapIfValid(repository::save) // invoke business operations on the entity (only if no errors have been accumlated so far)
.mapIfValid(representations::toRepresentation) // produce representation from result of the business operation
.concludeIfValid(ResponseEntity::ok); // prepare response
The fundamental idea of that is that processing steps can accumulate additional errors and only execute the mapIfValid(…) steps and eventually end up in concludeIfValid(…) if no errors have been accumulated until that. If at least one error has occurred, a ResponseEntity with the Errors instance (to be processed by an HttpMessageConverter) would be created. A 404 Not Found would be produced if the notFoundIf(…) guard kicks in.
As this is a highly opinionated way of processing requests, I think a reasonable balance is to allow users to declare such a type in a way that Spring MVC can actually produce it as handler method argument. The current arrangement of HandlerMethodArgumentResolvers unfortunately doesn't allow that.
A framework provided, unified type (like Payload shown above) plus a dedicated mapping step into user code space would solve that issue. I was wondering if, in case the user type could carry some annotation plus/or a dedicated factory method to be constructed from a Payload, that mapping step could be invoked by Spring MVC's argument resolution. E.g. a declaration of MappedPayload like this:
class MappedPayload<T> {
static <T> MappedPayload<T> of(Payload<T> payload) { … }
…
}
would allow MappedPayload be immediately used like this:
class MyController {
@PostMapping("/comments")
HttpEntity<?> postComment(MappedPayload<CommentDto> payload) { … }
}
Comment From: odrotbohm
A prototypical implementation of this exists in spring-playground.
Comment From: rstoyanchev
MappedPayloads is the interesting part but you're saying that would be in application code. If Payload is nothing more than a simple container of the body and Errors that is immediately passed to MappedPayloads, I'm not sure it makes that much difference whether it's done through one or two arguments.
I'm also wondering if this validation workflow could somehow be used in WebFlux or WebMvc functional endpoint APIs where currently there is no data binding but this is something likely to be done #25943 and some kind of validation goes along with data binding.
Comment From: odrotbohm
Thanks for the feedback, Rossen. There are a couple of aspects to this problem. One thing the Payload abstraction buys us is that indeed the declaration is shorter. There's also no way to forget @Valid or mess up the order of the arguments that is required but not really well-known (Errors having to immediately follow the @RequestBody parameter).
The final thing that I ran into is a bit of aesthetic nature: the multiple documents "pollute" the variable space within the method. In MappedPayloads, methods exist that take (Bi)Functions that provide access to the Errors backing the MappedPayloads instance. The problem is twofold: you all of a sudden have two Errors to refer to (the one handed into the callback, one from the handler method parameter) and thus also a naming issue as only one of the two can be named errors nicely. The naming "problem" also occurs for the payload object.
Regarding MappedPayloads being part of the application / library code: I'd be happy to see MappedPayloads and its API to be integrated into Framework. I never really properly thought through how it or something similar would make sense in alternative programming models like WebFlux or Web.fn. If it does, even better. When I spoke to Jürgen about it, it felt like a big commitment to that style of writing controller method implementations. So, I thought that a less invasive way was to actually allow users to plug in that or a similar abstraction. Currently, that is not possible at all, as the HandlerMethodArgument arrangement doesn't really allow to access the standard payload binding programmatically. I thought that rather providing a resolution mechanism for a less opinionated type (Payload in the example above) plus the ability to plug a translator into a more opinionated type would limit the commitment that Framework would have to give regarding a certain handler method implementation style but still allows third parties to plug in something more opinionated exposed to the user.
Comment From: rstoyanchev
Note that "Payload" has a strong association with spring-messaging and messaging projects like Spring Integration and others. The Message type has headers and a payload. The terminology is used pervasively throughout and there is even a @Payload annotation. I don't think an overlap would be ideal even if not incorrect. Request and response body is what we stick to throughout our web modules.
Naming aside, it's true that the wrapper makes the signature shorter, but I'm not sure that's an advantage. It also obscures things slightly by introducing an additional type and also requiring access to the contained body and errors within the method. So unless one builds a MappedPayload API or similar, by comparison it is more convenient to inject and access the body and errors as separate arguments even if the signature is slightly longer.
The other concern is consistency. We have also @ModelAttribute that can be followed for Errors or RedirectAttributes. I think we would need to anticipate having similar wrappers there as well. On the WebFlux side we support injecting the body as Mono in which case you get the errors through the Mono and then you can't add Errors.
Comment From: odrotbohm
Re "Payload" – I've used the term as a placeholder. I am not attached to it at all. I tried to express a "more generic flavor of MappedPayload. The latter term was used in a customer project to express the inbound bound data, which would have come from either an @RequestBody mapped object or one accumulating form-style parameters bound from a complex @ModelAttribute object. It abstracted "what the client sent".
Re two arguments VS. one – I respectfully disagree here. The type is expressing the concept of the bound payload plus what's wrong with it. Invocation of business logic might add up errors, and the interaction patterns with both these elements can nicely by expressed as dedicated operations, as shown in the implementation of MappedPayloads. I.e. there's high cohesion between the two concepts. It's tedious to always hand two things around as a pair when the two of them actually form a dedicated, single concept. A concept that is explicit and expressed as a type. Introducing that type eliminates the entire problem of correct parameter ordering (Errors having to follow the @RequestBody or @ModelAttribute type). Also, the "duplicate instance reference" issues still stands. It's not just a question of pure naming (like: finding a good name for a single variable), but avoiding having multiple pointers to the same instance and the naming collisions stemming from that (like: I need to find a second name for the same but different thing).
My primary objective when I filed this ticket was to be able to provide something like MappedPayload as a library developer, being able to make use of accessing the bound request body or parameters and Errors instance and providing that as a single argument to the controller method that then exposes a high-level API to compose a request processing pipeline. That's currently not possible. If there's a way to enable this without exposing a new framework supported handler method parameter type, I wouldn't mind that either. If e.g. the resolution of @RequestBody/@ModelAttribute was aware of a PayloadWrapper<T> (name just a placeholder again) marker interface, proceeded as if they were dealing with a T and finally wrapped that into the type implementing PW by using a factory method convention before handing the value into the controller, that'd work for me, too.
// Library provided
class MappedPayload<T> implements PayloadWrapper<T> { // <- PW Spring MVC interface
public static MappedPayload<T> of(T t, Errors errors) { … }
}
@PostMapping
HttpEntity<?> post(@RequestBody MappedPayload<Sample> payload) { … }
That'd address your concern regarding supporting both @RB and @MA as those would still have to be explicit, or – to phrase it better – follow the already existing rules (@MA being implicitly used for arguments not handled by an HMC).
Comment From: rstoyanchev
Team Decision: this makes sense when combined together with the additional fluent API. If it's only the wrapping, it doesn't make sense to do within the framework since the first thing anyone would have to do is to unwrap the arguments.
Ideally, an external library would be able to create a custom argument resolver that exposes directly the entry point for the fluent API without the need for any wrapping. As far as I can see, you could extend or delegate to RequestResponseBodyMethodProcessor and catch MethodArgumentNotValidException in case of validation errors. If there is anything we can do to improve on that end, it would be the direction we would take.
Comment From: rstoyanchev
Closing for now, but feel free to comment and we can re-open as needed.