Specify overwrite mechanism for form methods #72

erdlet · 2022-01-04T15:40:13Z

Hi all,

I just had a little bit of time to begin with the specification of HTML form method overwrites. We already have this mechanism in Krazo, so I used the implementation there as inspiration for the default implementation in the spec. As this is my first addition to the spec, I'm pretty sure there are a lot of things I can do better.

Please let me know what you think about this draft @eclipse-ee4j/ee4j-mvc-committers and anyone else interested.

ivargrimstad

LGTM

chkal

@erdlet Thank you very much for working on this. I would really love to see this in the spec.

However, I want to share some thoughts. To be honest, I'm not completely sure if we really need this SPI interface which people can implement to provide custom behavior. Isn't the default implementation everything people actually require in real work projects?

When I thought about this feature before, I was actually thinking about just providing:

A configuration parameter which enables or disables the form method override feature-
A configuration parameter to customize the name of the request parameter which contains the HTTP verb.

And that's all. Isn't this sufficient for 99% of all use cases?

And if people really want to have some kind of fancy custom behavior, they can actually implement it with a standard JAX-RS ContainerRequestFilter, which has just a few lines of code. Even the default mechanism which MVC implementations provide boils down to something like this:

@Provider
@PreMatching
public class MethodContainerRequestFilter implements ContainerRequestFilter {

  @Override
  public void filter( ContainerRequestContext requestContext ) throws IOException {

    String method = requestContext.getHeaderString( "X-Method" );
    if( method != null ) {
      requestContext.setMethod( method );
    }

  }

}

So I wonder if this SPI isn't an overkill for this simple feature?

@eclipse-ee4j/ee4j-mvc-committers Any other thoughts?

chkal · 2022-01-09T11:36:57Z

api/src/main/java/jakarta/mvc/form/FormMethodOverwriter.java

+    /**
+     * The name of the hidden form input to get the targeted HTTP method.
+     */
+    String HIDDEN_FIELD_NAME = "_method";


Two comments:

Does anyone know if there is some kind of "de-facto standard" for the name of this parameter? AFAIK other frameworks support this mechanism as well and if there is some kind of standard name, it would be cool to use it.

Maybe we should allow to customize the name of the form field via some configuration parameter?

All frameworks I know use _method (especially Spring), so this name should be the de-facto standard. I added a possibility to overwrite this name in my new draft.

chkal · 2022-01-09T11:39:06Z

spec/src/main/asciidoc/chapters/form-method-overwrite.asciidoc

+
+Jakarta MVC defines the term _form method overwrite_ as mechanism being responsible for changing the HTTP request's method to something different than `GET` or `POST`. 
+
+The _form method overwrite_ MUST happend exactly once per request as described in <form_method_overwrite_resolving_algorithm>> before the `Controller` is resolved.


Is there a < missing?

Yes. Thanks for that catch :)

erdlet · 2022-01-09T12:16:03Z

Thanks for your feedback @chkal. After thinking a few days about that proposal I came to the same conclusion that the SPI would be an overkill.

So I think I'll change the proposal so that only the current default mechanism is required as well as the configuration parameters.

chkal · 2022-01-09T14:54:52Z

So I think I'll change the proposal so that only the current default mechanism is required as well as the configuration parameters.

+1 from my side.

@eclipse-ee4j/ee4j-mvc-committers It would be great to hear other thoughts as well.

erdlet · 2022-01-16T10:41:49Z

Hi @eclipse-ee4j/ee4j-mvc-committers,

I changed the proposal according to my and @chkal's thoughts. I think this draft is enough to start and will cover 99% of the common use-cases. Would be great to get some feedback.

@jelemux Maybe you want to give some feedback too?

jelemux · 2022-01-19T13:17:52Z

So if I understand correctly, we can have a form which sends a POST but because of the hidden input MVC interprets it for example like a PATCH. Therefore a form like that could call a controller with a @PATCH method. Is this correct?
If so, this looks good to me.

Will there be TCK tests for this?

erdlet · 2022-01-19T14:29:10Z

Thanks @jelemux for your feedback!

So if I understand correctly, we can have a form which sends a POST but because of the hidden input MVC interprets it for example like a PATCH. Therefore a form like that could call a controller with a @patch method. Is this correct?

Absolutely correct 👍

Will there be TCK tests for this?

Yes, I just want to have a final draft for this before I start to work on the tests to avoid double work in case of changes. I plan to test the behavior of the config options and usage of a different hidden field name. Think it will sum up to three or four tests.

chkal

I've finally found some time for reviewing this. Sorry for the delay.

I added quite some comments. But mostly minor stuff. I also added suggestions to simplify the process of applying them if we decide to change the corresponding parts.

spec/src/main/asciidoc/chapters/form-method-overwrite.asciidoc

api/src/main/java/jakarta/mvc/form/FormMethodOverwriter.java

erdlet · 2022-02-15T06:37:10Z

Hi all,

I'm afraid there won't be more feedback. So if this PR is OK for the reviewers, it would be great when @chkal or @ivargrimstad merge it. After that I'll implement the TCK tests and adapt Krazo core.

chkal

I added a few more comments. I also included proposed changes for all my comments to make it easier to apply them if we go with them.

Sorry for the delay in my review and for even more comments. But I guess it would be preferable to have it polished before merging it. :-)

api/src/main/java/jakarta/mvc/form/FormMethodOverwriter.java

spec/src/main/asciidoc/chapters/form-method-overwrite.asciidoc

see: #39

erdlet · 2022-02-21T18:30:27Z

@chkal thanks for the feedback. I hope the spec chapter is fine now :)

chkal · 2022-02-27T08:23:42Z

Awesome! Thanks a lot for applying the changes.

One last thing. I think the new chapter is still missing in the mvc-spec.asciidoc file and therefore not included in the resulting PDF, or am I missing something. Sorry, just noticed this while looking at all the changes in the PR.

see: #39

erdlet · 2022-02-27T10:36:16Z

Thanks @chkal, I really missed to add the new chapter 👍

chkal

Awesome! Thanks!

@ivargrimstad @jelemux Could you check the PR again and approve? Thanks!

chkal · 2022-02-27T11:58:47Z

@ivargrimstad @jelemux Ah, sorry. Looks like your approval is still valid. Never mind. :-)

erdlet added this to the 2.1 milestone Jan 4, 2022

erdlet requested review from ivargrimstad and chkal January 4, 2022 15:40

erdlet self-assigned this Jan 4, 2022

ivargrimstad approved these changes Jan 5, 2022

View reviewed changes

chkal reviewed Jan 9, 2022

View reviewed changes

jelemux approved these changes Jan 20, 2022

View reviewed changes

chkal reviewed Jan 22, 2022

View reviewed changes

chkal reviewed Feb 20, 2022

View reviewed changes

Specify overwrite mechanism for form methods

127cbb0

see: #39

Include new chapter in spec file

0d4fcb4

see: #39

chkal approved these changes Feb 27, 2022

View reviewed changes

chkal merged commit 7760d65 into jakartaee:master Mar 1, 2022

ivargrimstad mentioned this pull request Sep 5, 2022

Idea: Standardized way to override HTTP methods in forms #39

Closed

erdlet mentioned this pull request Sep 13, 2022

Add accessor for form method overwrite field name #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify overwrite mechanism for form methods #72

Specify overwrite mechanism for form methods #72

erdlet commented Jan 4, 2022

ivargrimstad left a comment

chkal left a comment

chkal Jan 9, 2022

erdlet Jan 16, 2022

chkal Jan 9, 2022

erdlet Jan 16, 2022

erdlet commented Jan 9, 2022

chkal commented Jan 9, 2022

erdlet commented Jan 16, 2022

jelemux commented Jan 19, 2022 •

edited

Loading

erdlet commented Jan 19, 2022

chkal left a comment

erdlet commented Feb 15, 2022

chkal left a comment

erdlet commented Feb 21, 2022

chkal commented Feb 27, 2022

erdlet commented Feb 27, 2022

chkal left a comment

chkal commented Feb 27, 2022


		Jakarta MVC defines the term _form method overwrite_ as mechanism being responsible for changing the HTTP request's method to something different than `GET` or `POST`.

		The _form method overwrite_ MUST happend exactly once per request as described in <form_method_overwrite_resolving_algorithm>> before the `Controller` is resolved.

Specify overwrite mechanism for form methods #72

Specify overwrite mechanism for form methods #72

Conversation

erdlet commented Jan 4, 2022

ivargrimstad left a comment

Choose a reason for hiding this comment

chkal left a comment

Choose a reason for hiding this comment

chkal Jan 9, 2022

Choose a reason for hiding this comment

erdlet Jan 16, 2022

Choose a reason for hiding this comment

chkal Jan 9, 2022

Choose a reason for hiding this comment

erdlet Jan 16, 2022

Choose a reason for hiding this comment

erdlet commented Jan 9, 2022

chkal commented Jan 9, 2022

erdlet commented Jan 16, 2022

jelemux commented Jan 19, 2022 • edited Loading

erdlet commented Jan 19, 2022

chkal left a comment

Choose a reason for hiding this comment

erdlet commented Feb 15, 2022

chkal left a comment

Choose a reason for hiding this comment

erdlet commented Feb 21, 2022

chkal commented Feb 27, 2022

erdlet commented Feb 27, 2022

chkal left a comment

Choose a reason for hiding this comment

chkal commented Feb 27, 2022

jelemux commented Jan 19, 2022 •

edited

Loading