Skip to content
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

@ModelAttribute binding defined globally for particular attribute rather than per method invocation [SPR-16083] #20632

Closed
spring-issuemaster opened this issue Oct 17, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Oct 17, 2017

Brice Roncace opened SPR-16083 and commented

Given this test controller (and java bean):

@Controller
public class TestController {

  public static class Bean {
    private Long id;

    public Long getId() {
      return id;
    }

    public void setId(Long id) {
      this.id = id;
    }

    @Override
    public String toString() {
      return "Bean{" + "id=" + id + '}';
    }
  }

  @ModelAttribute
  public Bean prepareBean() {
    return new Bean();
  }

  @ModelAttribute
  public void prepareBean2(@ModelAttribute(binding=false) Bean bean) {
    System.out.println("prepareBean2: " + bean);
  }

  @GetMapping("/bindTest")
  public String editBean(@ModelAttribute(binding=true) Bean bean, BindingResult br) {
    System.out.println("Inside editBean: " + bean);
    System.out.println(br.getAllErrors());
    return "index";
  }
}

When exercising this controller with bindTest?id=1 no binding will occur because the prepareBean2 method disables binding for the bean model attribute even though a subsequent use of the @ModelAttribute in the editBean method expects binding to occur.

The unsatisfactory workarounds are to allow binding and insert an ignored BindingResult parameter in the prepareBean2 method so that if binding errors occur, they can be handled in the editBean method where they are expected:

  @ModelAttribute
  public Bean prepareBean() {
    return new Bean();
  }

  @ModelAttribute
  public void prepareBean2(@ModelAttribute Bean bean, BindingResult ignore) {
    System.out.println("prepareBean2: " + bean);
  }

  @GetMapping("/bindTest")
  public String editBean(@ModelAttribute Bean bean, BindingResult br) {
    System.out.println("Inside editBean: " + bean);
    System.out.println(br.getAllErrors());
    return "index";
  }

Or pull the bean out of the model to prevent binding in the prepareBean2 method:

  @ModelAttribute
  public Bean prepareBean() {
    return new Bean();
  }

  @ModelAttribute
  public void prepareBean2(Model m) {
    Object bean = m.asMap().get("bean");
    System.out.println("prepareBean2: " + bean);
  }

  @GetMapping("/bindTest")
  public String editBean(@ModelAttribute Bean bean, BindingResult br) {
    System.out.println("Inside editBean: " + bean);
    System.out.println(br.getAllErrors());
    return "login";
  }

Note: in a real-world example these two @ModelAttribute annotated methods would not appear in the same class (e.g. @ControllerAdvice would apply the initial prepareBean method).


Issue Links:

  • #17982 Prevent binding for @ModelAttribute

Referenced from: commits b0ae8f6, bec1fc1

Backported to: 4.3.13

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 17, 2017

Juergen Hoeller commented

At second glance, the current behavior is indeed an unintuitive mismatch with what has been declared in the handler methods. I'll see what we can do about this for 5.0.1 next week, and we'll also consider a backport to 4.3.13 eventually.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 18, 2017

Juergen Hoeller commented

This turns out to be easy enough to fix through detecting a re-enabled binding declaration in ModelAttributeMethodProcessor. Please give the upcoming 5.0.1.BUILD-SNAPSHOT or 4.3.13.BUILD-SNAPSHOT a try..

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 18, 2017

Brice Roncace commented

Verified that this is working as expected on the latest 4.3.13.BUILD-SNAPSHOT. Thank you, Juergen, for addressing this so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.