Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
odrotbohm opened this issue Dec 21, 2022 · 9 comments
Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement

Comments

@odrotbohm
Copy link
Member

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
Copy link
Contributor

rstoyanchev commented Jan 3, 2023

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 rstoyanchev added the status: declined A suggestion or change that we don't feel we should currently apply label Jan 24, 2023
@rstoyanchev
Copy link
Contributor

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.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) status: pending-design-work Needs design work before any code can be developed and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: declined A suggestion or change that we don't feel we should currently apply labels Jan 24, 2023
@rstoyanchev rstoyanchev added this to the 6.x Backlog milestone Jan 24, 2023
@rstoyanchev rstoyanchev added the in: test Issues in the test module label Jan 24, 2023
@odrotbohm
Copy link
Member Author

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.

@jhoeller jhoeller added the type: enhancement A general enhancement label Jan 11, 2024
@rstoyanchev
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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.

@patpatpat123

This comment has been minimized.

@sbrannen

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants