Fordward renders 404 error when should not #11102

maurociancio · 2018-10-07T22:52:29Z

I've created a sample project showing the problem: https://github.com/maurociancio/grails-3-forward-issue .

Given the following controller:

class TestController {

    def get() {
    }

    def post() {
      forward action: 'get'
    }
}

And the following UrlMappings:

class UrlMappings {

    static mappings = {
        name testMapping: '/test-mapping'(controller: 'test') {
          action = [GET: 'get', POST: 'post']
        }

        /*
        PLEASE NOTE THIS SNIPPET IS COMMENTED OUT
        "/$controller/$action?/$id?(.$format)?"{
            constraints {
                // apply constraints here
            }
        }*/

        "/"(view:"/index")
        "500"(view:'/error')
        "404"(view:'/notFound')
    }
}

And the following get.gsp:

<!doctype html>
<html>
<head>
    <meta name="layout" content="main"/>
    <title>Welcome to Grails</title>
</head>
<body>
  <g:form mapping="testMapping">
    <input type="submit" />
  </g:form>
</body>
</html>

When the submit button is pressed, a 404 is issued in the server. I expected that post() action forwards to get() action and the same view is showed again.

Task List

Steps to reproduce provided
Example that reproduces the problem uploaded to Github
Full description of the issue provided (see below)

Steps to Reproduce

Clone https://github.com/maurociancio/grails-3-forward-issue
$ grails run-app
Hit http://localhost:8080/test-mapping in the browser and click submit button.
404 is shown.

Expected Behaviour

I expected the same view to be showed.

Actual Behaviour

404 is shown.

Environment Information

Operating System: Ubuntu 16.04
Grails Version: Tested with 3.3.8 and 3.3.6
JDK Version: java version "1.8.0_101"
Java(TM) SE Runtime Environment (build 1.8.0_101-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.101-b13, mixed mode)

Example Application

https://github.com/maurociancio/grails-3-forward-issue

The text was updated successfully, but these errors were encountered:

osscontributor · 2018-10-08T14:24:46Z

Fordward renders 404 error when should not

I don't think this has anything to do with a forward. The issue that your GSP is rendering a form that looks like this, which is invalid:

<form action="/test-mapping" method="post" >
    <input type="submit" />
</form>

I haven't looked into why that is happening, but I don't think it is related to your forward.

maurociancio · 2018-10-08T17:21:21Z

Hi @jeffbrown thanks for your reply. Why is that form invalid?
When I perform GET to http://localhost:8080/test-mapping I get the page (no error). When I submit the form (or I POST to http://localhost:8080/test-mapping) I get an 404.
Thanks!

osscontributor · 2018-10-08T18:36:17Z

@maurociancio Apologies. You are 100% correct. I mis-read it.

Sorry for the noise.

maurociancio · 2018-10-08T18:58:44Z

Hi @jeffbrown, no problem! Is this a bug or am I misusing the API?
Thanks again!

ilopmar · 2018-10-09T09:07:41Z

Hi @maurociancio I think this is a valid bug. The issue is that during the forward method, the url to forward to is calculated as /test/get (using controllerName + actionName) instead of using the entry in the UrlMappings /test-mapping.
If you uncomment the block

"/$controller/$action?/$id?(.$format)?"{
    constraints {
    }
}

It works.

I'm working on a fix as we speak.

maurociancio · 2018-10-09T14:12:06Z

Hey @ilopmar, thanks for your reply!
Please let me know when you have the fix so I can test it!
Thanks again!

ilopmar · 2018-10-10T12:58:58Z

Hi @maurociancio, can you please why you are doing a forward instead of a redirect? After debugging this a little bit more I'm not really sure if this is a bug or not, so I would like to know what you try to achieve with this.

maurociancio · 2018-10-10T13:10:25Z

Hi @ilopmar! Of course, the example I provided was pretty small.
I use that pattern a lot when trying to create new entities in my app. For example:

class Controller {
  def create() {
    [command: request.command ?: new MyCommand()]
  }

  def doCreate(MyCommand cmd) {
    if (!cmd.hasErrors()) {
      new Entity().save()
      // redirect to somewhere else
    } else {
      forward action: 'create', model: [command: cmd]
    }
  }
}

So, when the validation fails in doCreate() I forward to the create() action so I can show the form again and provide feedback to the user about the validation errors. If I use redirect I would lose the command object between requests. Of course, I could render the create view again instead of forwarding, but I'd have to copy-paste the code in create() or extract a new method to have the duplicate code.

Is my example clear enough?
Thanks, let me know if you need a more complex demo app.

ilopmar · 2018-10-10T13:16:33Z

Why do you "lose the command object between request". You can do something like:

redirect action: 'create', model: [command: cmd]

Which is the same as you have in the else branch but using redirect instead of forward. Your create action already handles receiving a command.
What am I missing?

maurociancio · 2018-10-10T13:29:03Z

I see your point, but: are the meanings of redirect and forward the same? I mean, when you forward() there no serialization of your command object. In the same request the command object is passed from one action to another. The user does not see the params. I could pass "private" parameters between the two actions, and the user would not notice.

When you redirect() the command object has to be serialized in the query string, right? So I could potencially be leaking some private parameters between the two actions.

So, they are similar but the semantics are different, right?
Please let me know if I got something wrong.
Thanks!

ilopmar · 2018-10-10T13:52:34Z

Yes, you are right. When doing a redirect all the parameters are displayed on the browser and appended to the url query string.
I never used the pattern you mention before when I was developing Grails applications with GSPs. What I did was something like:

if (cmd.hasErrors) {
   render view: 'create', model:[cmd: cmd,...]
} else {
   redirect view: 'show', id: object.id
}

So, if there is an error in the validation, instead of forwarding I render a view and I can pass any parameter I want.

maurociancio · 2018-10-10T14:01:06Z

Hi @ilopmar!
Great. I could do that (I dont want to, because I'd have to review all the code), but given that I was migrating a grails 2 app to grails 3, and my example worked before, I reported this.
Is there any chance this could be fixed? Or maybe if I submit a pull request, will it get merged?
Thanks!

ilopmar · 2018-10-10T14:04:26Z

I'm still debugging this and tried a fix before but I broke other thing :-P
I haven't finished yet so I can't say it won't be fixed. If you submit a PR with tests we will review it and merge it. PR are always welcome :)

ilopmar · 2018-10-10T14:06:24Z

BTW, in the mean time, if you don't want to change your code you can enable the default Url Mappings or create specific entries for the affected actions.

maurociancio · 2018-10-10T14:08:11Z

Hi @ilopmar! Okay! Thanks!
In what branch are you working on?

ilopmar · 2018-10-10T14:11:00Z

I haven't pushed anything yet so feel free to fork the repo and create your own branch.

maurociancio · 2018-10-10T14:16:48Z

Haha, I could use some inspiration!

ilopmar · 2018-10-10T15:56:27Z

Hi @maurociancio. I've been discussing this issue with @jameskleeh and this is finally not a bug per-se.
What I'm going to do is fix it to allow execute a forward using the same http method. So if you hit an action using GET you will be allowed to forward the execution to another action reachable using GET.
In your use case, when forwarding from POST to GET, you will have an error different than a 404 but you will need to change your code to do a redirect or render a view depending on the case.

maurociancio · 2018-10-10T18:02:25Z

Hi @ilopmar! Thanks for your reply!
I don't fully understand why this is not a bug. Would you please explain me?
Thanks!

jameskleeh · 2018-10-10T18:08:15Z

@maurociancio Because you can't change the method in a forward. The forward to an action will look up the url mapping for the action and forward to that URL. Since the URL is the same for both of your actions, the original action will be invoked because the action you've attempted to forward to only responds to GET and the original request is a POST. That will lead to a stackoverflow.

jameskleeh · 2018-10-10T18:10:11Z

POST /test-mapping -> forward determines URL for action get() -> dispatch to /test-mapping (still POST!) -> stackoverflow

maurociancio · 2018-10-10T18:13:09Z

Haha, that makes sense now! Thanks for your time!

Fix regression due to the fix of #11102

ilopmar self-assigned this Oct 8, 2018

ilopmar added the status: in progress label Oct 9, 2018

ilopmar mentioned this issue Oct 11, 2018

Add an additional lookup for urls when defining multiple http methods… #11109

Merged

ilopmar added status: pr submitted and removed status: in progress labels Oct 11, 2018

ilopmar added this to the grails-3.3.9 milestone Oct 11, 2018

graemerocher closed this as completed in e90ad6c Oct 11, 2018

ilopmar added a commit that referenced this issue Oct 19, 2018

Fix regression due to the fix of #11102

9ca3eef

graemerocher added a commit that referenced this issue Oct 26, 2018

Merge pull request #11124 from grails/fix-regression-grails-gsp

cb93c27

Fix regression due to the fix of #11102

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fordward renders 404 error when should not #11102

Fordward renders 404 error when should not #11102

maurociancio commented Oct 7, 2018

osscontributor commented Oct 8, 2018

maurociancio commented Oct 8, 2018

osscontributor commented Oct 8, 2018

maurociancio commented Oct 8, 2018

ilopmar commented Oct 9, 2018

maurociancio commented Oct 9, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

jameskleeh commented Oct 10, 2018

jameskleeh commented Oct 10, 2018

maurociancio commented Oct 10, 2018

Fordward renders 404 error when should not #11102

Fordward renders 404 error when should not #11102

Comments

maurociancio commented Oct 7, 2018

Task List

Steps to Reproduce

Expected Behaviour

Actual Behaviour

Environment Information

Example Application

osscontributor commented Oct 8, 2018

maurociancio commented Oct 8, 2018

osscontributor commented Oct 8, 2018

maurociancio commented Oct 8, 2018

ilopmar commented Oct 9, 2018

maurociancio commented Oct 9, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

ilopmar commented Oct 10, 2018

maurociancio commented Oct 10, 2018

jameskleeh commented Oct 10, 2018

jameskleeh commented Oct 10, 2018

maurociancio commented Oct 10, 2018