RestfulController problem

[Jeff Scott Brown]

I think http://jira.grails.org/browse/GRAILS-11354 is a fairly serious problem.  I think controllers which extends RestfulController are broken in a bad way in 2.3.8, and it is my fault (see below).

The createResource method in RestfulController looks like this:


    protected T createResource(Map params) {
        resource.newInstance(params)
    }

A problem with this is that if the request contains a body, the params is an empty Map so the resource is not populated with with the expected values (the values from the body).

I haven’t gone through adequate testing yet but at least for simple cases, something like this works:

    protected T createResource(Map params) {
        T t = resource.newInstance()
        bindData t, request
        t
    }

This all worked in 2.3.7 but is broken in 2.3.8.  The commit that broke this was made by me.  See https://github.com/grails/grails-core/commit/498853e4aca0efefd6c8a4e1a2befe8da77896f8.  When I made that change I discussed it with Graeme and at that time Graeme said that he was sure that when he added the code I was removing there that there was a reason, but he couldn’t remember what it was.  It seems he was right, but I think that the code I removed is probably not really what we want.  The problem with that code is that if data binding is being done with params during a request that has a body, the params are ignored even though the code that initiated the binding explicitly bound using params, not the body.  See the unit tests in the commit linked above.  I think those are valid and describe what should happen for those scenarios.

I think it is probably incorrect that RestfulController does binding with params.  I think if you bind with params, params should be used.  I think if you bind with request and the request has the body, the body should be used, otherwise params should be used.

Related to this is http://jira.grails.org/browse/GRAILS-11263 which requests that we use both the body and params.  I think that is easy enough to do but we need to decide which takes precedence if they both contain the same keys.

What say ye of all/any of that?



JSB

—
Jeff Scott Brown
***@gopivotal.com

Find The Cause ~ Find The Cure
http://www.autismspeaks.org/



---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[Peter Ledbrook]

Hi Jeff,

I think it is probably incorrect that RestfulController does binding with params. I think if you bind with params, params should be used. I think if you bind with request and the request has the body, the body should be used, otherwise params should be used.

This isn't particularly clear due to the nature of the `params` map in
Grails. I think it would be useful to the conversation to distinguish
between the HTTP side of things (URL parameters and request body) and
the Grails side of things (`params` map and request body). I think
this is particularly important because `params` used to be an
amalgamation of the URL parameters and data from the request body. Has
this changed? Does `params` only ever contain URL parameters now?

As far as URL parameters vs request body goes, I would be tempted to
return a 400 if there is any duplication between the two. Either that
or ignore URL parameters for POSTs and PUTs, but that seems too
limiting.

Cheers,

Peter

[Graeme Rocher]

I don't understand why you would want to bind from request parameters
and a body at the same time, as far as I'm concerned ignoring the
parameters was the right behaviour (the way it was before) and the two
should be mutually exclusive.

Cheers

I think http://jira.grails.org/browse/GRAILS-11354 is a fairly serious problem. I think controllers which extends RestfulController are broken in a bad way in 2.3.8, and it is my fault (see below).
protected T createResource(Map params) {
resource.newInstance(params)
}
A problem with this is that if the request contains a body, the params is an empty Map so the resource is not populated with with the expected values (the values from the body).
protected T createResource(Map params) {
T t = resource.newInstance()
bindData t, request
t
}
This all worked in 2.3.7 but is broken in 2.3.8. The commit that broke this was made by me. See https://github.com/grails/grails-core/commit/498853e4aca0efefd6c8a4e1a2befe8da77896f8. When I made that change I discussed it with Graeme and at that time Graeme said that he was sure that when he added the code I was removing there that there was a reason, but he couldn’t remember what it was. It seems he was right, but I think that the code I removed is probably not really what we want. The problem with that code is that if data binding is being done with params during a request that has a body, the params are ignored even though the code that initiated the binding explicitly bound using params, not the body. See the unit tests in the commit linked above. I think those are valid and describe what should happen for those scenarios.
I think it is probably incorrect that RestfulController does binding with params. I think if you bind with params, params should be used. I think if you bind with request and the request has the body, the body should be used, otherwise params should be used.
Related to this is http://jira.grails.org/browse/GRAILS-11263 which requests that we use both the body and params. I think that is easy enough to do but we need to decide which takes precedence if they both contain the same keys.
What say ye of all/any of that?
JSB
—
Jeff Scott Brown
Find The Cause ~ Find The Cure
http://www.autismspeaks.org/
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

[Jeff Scott Brown]

Post by Graeme Rocher
I don't understand why you would want to bind from request parameters
and a body at the same time, as far as I'm concerned ignoring the
parameters was the right behaviour (the way it was before) and the two
should be mutually exclusive.
 

For the "as far as I'm concerned ignoring the parameters was the right behaviour" I would like to address that and if we still don't agree then of course we can just go back to the old behavior.  Before doing that I want to restate how I see this.

For the purposes of this conversation lets say that there 3 things that the data binder might use as the source for the binding process when you do something like obj.properties=[something goes here].  

1.) the request parameters
2.) the request body
3.) the request

In 2.3.8 when you do that kind of data binding if you specify an instance of GrailsParameterMap as the RHS, the data binding process uses the request parameters when doing the binding.  That part makes sense to me and I hadn't considered that might be controversial.  If you specify the request object as the RHS then 1 of 2 things can happen.  If the request has a body, the data binding process will use the request body when doing the binding.  If the request does not have a body then the data binding process will use the request parameters when doing the binding.

In 2.3.7 when you do that kind of data binding if you specify an instance of GrailsParameterMap as the RHS, if the request has a body, the parameter map that was explicitly expressed as the thing I wanted to do data binding with, that thing is ignored by the data binding process and the request body is used.  That is the behavior that I think is suspect.  That behavior seems to make sense if the request itself was specified as the RHS but not if the GrailsParameterMap was specified.

There are a lot of things we could do with this.  3 of those options that should be considered:

1.) Put the old behavior back in place and move on.
2.) Leave the new behavior in place and change RestfulController so it expects the new behavior.
3.) Put the old behavior back in place for 2.3.9 and for 2.4 use the new behavior, change RestfulController so it expects the new behavior and make the change in behavior a top level point of attention in the release notes.

At this point I am open to being convinced otherwise but I think #3 probably makes the most sense.  What do you all think?

Thanks for taking time and providing input.




JSB

—
Jeff Scott Brown
***@gopivotal.com

Find The Cause ~ Find The Cure
http://www.autismspeaks.org/



---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[Graeme Rocher]

Considering this a breaking change in a minor release I vote for 1) or 3)

Cheers

Post by Graeme Rocher
I don't understand why you would want to bind from request parameters
and a body at the same time, as far as I'm concerned ignoring the
parameters was the right behaviour (the way it was before) and the two
should be mutually exclusive.

For the "as far as I'm concerned ignoring the parameters was the right behaviour" I would like to address that and if we still don't agree then of course we can just go back to the old behavior. Before doing that I want to restate how I see this.
For the purposes of this conversation lets say that there 3 things that the data binder might use as the source for the binding process when you do something like obj.properties=[something goes here].
1.) the request parameters
2.) the request body
3.) the request
In 2.3.8 when you do that kind of data binding if you specify an instance of GrailsParameterMap as the RHS, the data binding process uses the request parameters when doing the binding. That part makes sense to me and I hadn't considered that might be controversial. If you specify the request object as the RHS then 1 of 2 things can happen. If the request has a body, the data binding process will use the request body when doing the binding. If the request does not have a body then the data binding process will use the request parameters when doing the binding.
In 2.3.7 when you do that kind of data binding if you specify an instance of GrailsParameterMap as the RHS, if the request has a body, the parameter map that was explicitly expressed as the thing I wanted to do data binding with, that thing is ignored by the data binding process and the request body is used. That is the behavior that I think is suspect. That behavior seems to make sense if the request itself was specified as the RHS but not if the GrailsParameterMap was specified.
1.) Put the old behavior back in place and move on.
2.) Leave the new behavior in place and change RestfulController so it expects the new behavior.
3.) Put the old behavior back in place for 2.3.9 and for 2.4 use the new behavior, change RestfulController so it expects the new behavior and make the change in behavior a top level point of attention in the release notes.
At this point I am open to being convinced otherwise but I think #3 probably makes the most sense. What do you all think?
Thanks for taking time and providing input.
JSB
—
Jeff Scott Brown
Find The Cause ~ Find The Cure
http://www.autismspeaks.org/
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

[Jeff Scott Brown]

Agreed that #2 is least desirable, so much so that it probably isn't really a candidate.

Let's discuss tomorrow.



JSB

Sent from my iPhone

Post by Graeme Rocher
Considering this a breaking change in a minor release I vote for 1) or 3)
Cheers

Post by Graeme Rocher
I don't understand why you would want to bind from request parameters
and a body at the same time, as far as I'm concerned ignoring the
parameters was the right behaviour (the way it was before) and the two
should be mutually exclusive.

For the "as far as I'm concerned ignoring the parameters was the right behaviour" I would like to address that and if we still don't agree then of course we can just go back to the old behavior. Before doing that I want to restate how I see this.
For the purposes of this conversation lets say that there 3 things that the data binder might use as the source for the binding process when you do something like obj.properties=[something goes here].
1.) the request parameters
2.) the request body
3.) the request
In 2.3.8 when you do that kind of data binding if you specify an instance of GrailsParameterMap as the RHS, the data binding process uses the request parameters when doing the binding. That part makes sense to me and I hadn't considered that might be controversial. If you specify the request object as the RHS then 1 of 2 things can happen. If the request has a body, the data binding process will use the request body when doing the binding. If the request does not have a body then the data binding process will use the request parameters when doing the binding.
In 2.3.7 when you do that kind of data binding if you specify an instance of GrailsParameterMap as the RHS, if the request has a body, the parameter map that was explicitly expressed as the thing I wanted to do data binding with, that thing is ignored by the data binding process and the request body is used. That is the behavior that I think is suspect. That behavior seems to make sense if the request itself was specified as the RHS but not if the GrailsParameterMap was specified.
1.) Put the old behavior back in place and move on.
2.) Leave the new behavior in place and change RestfulController so it expects the new behavior.
3.) Put the old behavior back in place for 2.3.9 and for 2.4 use the new behavior, change RestfulController so it expects the new behavior and make the change in behavior a top level point of attention in the release notes.
At this point I am open to being convinced otherwise but I think #3 probably makes the most sense. What do you all think?
Thanks for taking time and providing input.
JSB
—
Jeff Scott Brown
Find The Cause ~ Find The Cure
http://www.autismspeaks.org/
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

--
Graeme Rocher
Grails Project Lead
SpringSource
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

http://xircles.codehaus.org/manage_email

[Peter Ledbrook]

In 2.3.7 when you do that kind of data binding if you specify an instance of GrailsParameterMap as the RHS, if the request has a body, the parameter map that was explicitly expressed as the thing I wanted to do data binding with, that thing is ignored by the data binding process and the request body is used. That is the behavior that I think is suspect. That behavior seems to make sense if the request itself was specified as the RHS but not if the GrailsParameterMap was specified.

I agree with this. If I do

def book = new Book(params)

I expect it to bind everything in params, not ignore the map. I'd also
like to point out that with an x-www-form-urlencoded request, both the
request body *and* the URL query parameters are added to the `params`
map. Why should a JSON or XML request behave differently? On top of
that, the pre-2.3 REST implementation used ParameterCreationListeners
to parse JSON and XML into the `params` map, although this behaviour
could be switched on/off per URL.

1.) Put the old behavior back in place and move on.
2.) Leave the new behavior in place and change RestfulController so it expects the new behavior.
3.) Put the old behavior back in place for 2.3.9 and for 2.4 use the new behavior, change RestfulController so it expects the new behavior and make the change in behavior a top level point of attention in the release notes.

Probably #3, with RestfulController binding the request rather than
params. This does raise the question of what to do about command
objects. Should they bind to the request? Or to `params`? What if you
have one command object to bind to the request body and one for the
URL query parameters?

Perhaps Grails can give users some control by adding a parameter to
@Validateable that allows them to specify whether the class
automatically binds to the request body or the query parameters.

Cheers,

Peter