spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.67k stars 38.15k forks source link

Allow setting up a MockMVC form submission using an instance of the form-backing object #29728

Open odrotbohm opened 1 year ago

odrotbohm commented 1 year ago

To bind data submitted through HTML forms, a form-backing object is used as parameter of MVC controller methods.

@Controller
class MyController {

  @PostMapping
  String submit(@Valid @ModelAttribute MyForm form, Errors errors) {

  }
}

class MyForm {
  // properties go here
}

In a MockMVC integration test, to issue a request submitting that form, currently one would have to use the ….param(…) methods to populate the request body as plain Strings, also making sure that e.g. the same formatting ruled are used as are for binding and form value rendering. Also, one has to replicate property names as Strings for the keys handed into the ….param(…) method, which is not refactoring-friendly.

I would appreciate it, if instead, one could just call post(…).form(myFormBackingInstance) so that one could just populate the object and get the values added as requests parameters in matching format and also get the content type configured accordingly.

rstoyanchev commented 1 year ago

In data binding, we have a specific list of name-value pairs (parameters) to go by. In doing the reverse, i.e. extracting name-value pairs from an Object, we could create more parameters than necessary, so this could be convenient for some cases where it fits the Object structure, but not as well in others where the form object has properties that shouldn't be included.

I can see the convenience this aims at, but even with a view template one has to list the name-value pairs. I think tests should mirror that and set up the parameters as name-value pairs as well. An application could create a utility to simplify turning an Object into name value pairs, if it works for local assumptions, but such a utility would be better off in the application's own test code.

rstoyanchev commented 1 year ago

To summarize, given an Object, currently we don't know which properties are for use in data binding. Typically we work the other way around from the list of request parameters, overlayed with allowed/disallowed fields. We do have a few issues, including #12403, #18408, #18012, and #23618, that we intend to explore for 6.1 that would make it possible to make such decisions, and at that point this would become straight forward.

odrotbohm commented 1 year ago

Thanks for reconsidering, @rstoyanchev. I wouldn't expect any particular smarts built into this, especially for folks reusing complex domain types as form backing objects. I was rather thinking of dedicated form objects. Something similar to record Person(String firstname, String lastname, LocalDate birthday, …) {}. If code like that was supported out of the box, it might even lead to more folks using dedicated form backing types.

rstoyanchev commented 9 months ago

@odrotbohm you last comment makes sense, but how would we draw a line? What would be the requirement for an input object that is supported vs one that is not? We would need to state it and also be able to differentiate make a clear difference and reject what isn't supported.

For additional context, we have a related request for a similar feature for @HttpExchange in #32142.

odrotbohm commented 9 months ago

What do you think of aligning with the incoming data binding? In that, there is some strategy to determine which properties to bind, and a from-string-conversion approach that fails for types that do not have a dedicated converter registered. PropertyDescriptors work both ways. Further, dedicated DTO types will be already designed for data binding and thus – to properly work in production code – will need to be either conservative in their usage of complex types or have dedicated converters present for binding at least.

rstoyanchev commented 9 months ago

The main strategy for data binding has been the allowedFields and disallowedFields on DataBinder, but I'm not sure that would be very convenient here. More recently, constructor binding provides a practical alternative as it declares the exact inputs to use vs considering every input that may match some property. Doing the reverse here would mean using constructor args (and also recursively on objects with constructor args of their own) to decide what properties are eligible to send.

odrotbohm commented 9 months ago

I was rather thinking about the getters exposed (the read side of a PD) as those are usually the ones that are used to populate the form elements in Thymeleaf templates etc. Especially in template-rendered UIs (in contrast to JSON-based APIs) it's not unfair to assume symmetry between what's rendered and what's bound, which in turn makes records a pretty nice way to represent such data. It would be nice if that convenience would extend into the test cases as well.