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

More robust solution for form validation #179

Open
hantsy opened this issue May 16, 2020 · 9 comments
Open

More robust solution for form validation #179

hantsy opened this issue May 16, 2020 · 9 comments
Labels
enhancement New feature or request

Comments

@hantsy
Copy link

hantsy commented May 16, 2020

In the Spring Web MVC there are some helpers to improve the form data binding and validation.

  1. In krazo, when the validation is failed, we have to rebuild the from data in the request. see my sample TaskController.java#L94-L104, but in Spring WebMVC, it is not required, check TaskController.java#L94-L97.
  2. In Spring MVC, there are some helpers for the views, including, JSP tags, Thymeleaf tags, Freemarker Marco, etc, to help to build the form more effectively. If there any helpers planned in Krazo do the same job or add some methods like hasErrors(), getErrors() in MvcContext.
@chkal
Copy link
Contributor

chkal commented May 31, 2020

Hey @hantsy,

thanks for filing this issue and sorry for my delayed response.

Regarding (1): I wonder if this is more a spec issues. I agree that MVC is more explicit about storing data in the model. I agree that the Spring MVC example is simpler. However, I'm not completely sure how something like this could work in the MVC 1.0 world.

Regarding (2): Currently there are no plans for such enhancements. Perhaps you could have a look at MVC Toolbox which I created some time ago. It contains JSP tags for common functionality. Feedback welcome. :-)

@hantsy
Copy link
Author

hantsy commented Jun 3, 2020

@chkal

  1. I hope it can be improved in the next version(mvc spec).
  2. The MvcToolbox is cool.

@erdlet
Copy link
Member

erdlet commented Jun 12, 2020

Hi,

I wonder if we can just store the BindingResult in the model after the controller processing and in case there are errors. When I remember correct, this way is similar to Spring's. Or is there anything I miss in that thought, @chkal?

@chkal
Copy link
Contributor

chkal commented Jun 13, 2020

@erdlet Interesting idea. Or maybe BindingResult should be accessible via MvcContext. This way you could do something like #{mvc.bindingResult.failed}.

@erdlet
Copy link
Member

erdlet commented Jun 13, 2020

This would be a possibility too. But it would need an API change, whereas the simple "put the binding result into the model" can be implemented relatively fast without it. Nevertheless, I think we should specifiy the behavior of this mechanism better, because at the moment it is relatively loose.

@chkal When do you think are we able to extend the spec again? I'm afraid this has to wait until the migration to the EF?

@chkal
Copy link
Contributor

chkal commented Jun 13, 2020

This would be a possibility too. But it would need an API change, whereas the simple "put the binding result into the model" can be implemented relatively fast without it.

Yeah, I agree. Although AFAIK we could simply add getBindingResult() to MvcContextImpl (our implementation class of MvcContext) and it should work. However, I'm not sure if this is a good solution to "bypass" the spec this way.

Nevertheless, I think we should specifiy the behavior of this mechanism better, because at the moment it is relatively loose.

Which mechanism to you refer to exactly?

@chkal When do you think are we able to extend the spec again? I'm afraid this has to wait until the migration to the EF?

Good question. We already sent the initial contribution of the spec and TCK to the EF (see here and here). After getting the approval by the legal team, we can directly push the code to the GitHub repo. And actually we could start to work on the spec directly after that. Although we still have to decide how we want to continue. I guess it would make sense to first release MVC 2.0 with only the javax -> jakarta namespace update and without any additional changes. But all this is up to discussion.

@erdlet
Copy link
Member

erdlet commented Jun 13, 2020

Yeah, I agree. Although AFAIK we could simply add getBindingResult() to MvcContextImpl (our implementation class of MvcContext) and it should work. However, I'm not sure if this is a good solution to "bypass" the spec this way.

It would work, but I don't think a bypass of the spec is really good. It'd be more a hack than a good solution :/

Which mechanism to you refer to exactly?

The BindingResult. It's specified how it works before the controller is evaluated and that the implementation should warn, in case there is no action on the result, but nowhere is defined how you should handle it after the controller evaluation. So I think it would be good, if we can specify how to handle the BindingResult after we checked if it's failed or not. For example, we could define that it is written to the model or accessible through the MvcContext and then it is clear for all possible implementations.

I guess it would make sense to first release MVC 2.0 with only the javax -> jakarta namespace update and without any additional changes.

+1

@chkal
Copy link
Contributor

chkal commented Jun 14, 2020

It would work, but I don't think a bypass of the spec is really good. It'd be more a hack than a good solution :/

I could argue that this is simply extending the API provided by the spec. But as mentioned earlier, I'm not really sure if this is the way to go. :-)

The BindingResult. It's specified how it works before the controller is evaluated and that the implementation should warn, in case there is no action on the result, but nowhere is defined how you should handle it after the controller evaluation. So I think it would be good, if we can specify how to handle the BindingResult after we checked if it's failed or not. For example, we could define that it is written to the model or accessible through the MvcContext and then it is clear for all possible implementations.

OK, got it. TBH, I'm not sure if this is really required. IMO the binding result is primarily meant to be consumed by the controller which then can prepare the model in a way, that corresponding error messages can be displayed in the view. However, I agree that this requires some boilerplate code. But I guess we should discuss this on the new spec mailing list after the migration to EF has completed.

@erdlet
Copy link
Member

erdlet commented Jun 15, 2020

But I guess we should discuss this on the new spec mailing list after the migration to EF has completed.

I think that is the best idea :)

@erdlet erdlet added the enhancement New feature or request label Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants