Add hasBody to ngResource action configuration #12181

kaufholdr · 2015-06-21T09:50:33Z

The use of hasBody would allow developers of REST interfaces to decide if a request body is necessary or not on a per method basis.

The way ngResource currently handles request bodies in custom requests make the definition of custom requests pretty useless. It’s not possible to transfere a request body through a other requests than POST, PUT, PATCH. Or am I wrong?

It may be true that some browser implementations are also brocken when it comes to methods like DELETE but this broken behavior should not be enforced by angular.

Even as i read the #3207 after creating the patch its somehow picking up pmariduenas comment on creating a enableDeleteRequestBody. Its just the generic version for all custom requests.

caitp · 2015-06-21T09:57:06Z

I don't think custom http methods are necessarily recommended for REST services. This does need some consideration

kaufholdr · 2015-06-21T10:09:34Z

I thought that the decision of enabling custom methods in ngResource has already been made as it is possible right now.

The current behavior of silently dropping the content body for custom requests is just weired (it took me several hours of debugging to locate the problem in angularjs). For me it feels like a bug and not a feature.

Do you think that the inclusion of this patch could be harmfull to the functionality of ngResource?

caitp · 2015-06-21T10:15:25Z

Custom methods and custom http methods are different things, are there any other methods you want a body for? I know people have asked for bodies on DELETE requests

kaufholdr · 2015-06-21T10:33:53Z

I would not include a body in the DELETE request as I currently have no use for it but i would like to include a body in a LOGIN request which carries the user information in a JSON (not only username an pasword). Some other custom http methods where i would like to include a body are: CLOCKIN, CLOCKOUT, STORNO, ASSIGN, NOTIFY, DISABLE but they are all specific to the project I'm working on. That's why I would like to have a generic attribute on the action specification.

yathos · 2015-07-08T11:43:37Z

I created a modified bower-angular-resource version that includes the requested fix in version 1.4.1 https://github.com/yathos/bower-angular-resource If someone would like to test it.

kaufholdr · 2015-07-30T16:08:16Z

Please tell me if there is anything that I could do to get this pull request included.

Narretz · 2015-08-08T16:28:08Z

Honestly, I don't think custom HTTP methods are a big enough use case to warrant inclusion into the angular core. Especially since $resource is pretty much built for "normal" REST interfaces. If you are doing something special, the recommendation has always been to built your own wrapper around $http.

ElayneB · 2016-10-26T22:56:08Z

+1 for this feature. We have built a client on top of $resource and we consider DELETE with bodies to be part of "normal" rest interfaces.

The use case is bulk relationship delete.
E.G.
DELETE model/:id/relatedModel body: [relatedModelId, relatedModelId, ..]

kaufholdr · 2016-12-05T12:39:33Z

I'm still interested in getting this pull request integrated.

gkalpak · 2016-12-05T21:17:01Z

Let's consider this after 1.6.0 is released and decide what direction to go (either merge or close).
It is a minimal addition and doesn't seem to do any harm; plus it could take care of the recurring debate about whether delete requests should have a body or not.

gkalpak · 2016-12-05T21:21:36Z

src/ngResource/resource.js

@@ -640,7 +642,7 @@ angular.module('ngResource', ['ng']).
        };

        forEach(actions, function(action, name) {
-          var hasBody = /^(POST|PUT|PATCH)$/i.test(action.method);
+          var hasBody = /^(POST|PUT|PATCH)$/i.test(action.method) || action.hasBody === true;


I would find something like the following more intuitive:

var hasBody = action.hasBody === true || (action.hasBody !== false && /^(POST|PUT|PATCH)$/i.test(action.method));

Am I missing any reason why we shouldn't allow people to overwrite hasBody for POST, PUT, PATCH? (Not that they should.)

I'm fine with both versions of the statement. Should I modify the patch?

Unless you can think of a reason not to 😃

I had to like twice to see the error but the correct statement would be:
var hasBody = action.hasBody === true || (action.hasBody !== true && /^(POST|PUT|PATCH)$/i.test(action.method));
right?

I actually meant it the way I wrote it above. Your version is the same as:

var hasBody = action.hasBody === true || /^(POST|PUT|PATCH)$/i.test(action.method);

...which is not what we want.

googlebot · 2016-12-06T09:43:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-12-06T09:43:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-12-06T10:00:25Z

CLAs look good, thanks!

googlebot · 2016-12-06T10:00:25Z

CLAs look good, thanks!

kaufholdr · 2017-02-20T14:11:02Z

Is there anything more that I can do to get this feature included?

gkalpak

We also need tests for this.

gkalpak · 2017-02-21T17:48:10Z

src/ngResource/resource.js

@@ -193,6 +193,8 @@ function shallowClearAndCopy(src, dst) {
 *   - **`interceptor`** - `{Object=}` - The interceptor object has two optional methods -
 *     `response` and `responseError`. Both `response` and `responseError` interceptors get called
 *     with `http response` object. See {@link ng.$http $http interceptors}.
+ *   - **`hasBody`** - `{boolean}` - allows to specify if a request body should be included or not.
+ *     If not specified only POST,PUT and PATCH will have a request.


Nit: Missing whitespace after ,.

gkalpak · 2017-02-21T17:50:51Z

@kaufholdr, feel free to ping me once you've made the requested changes, so I can take another look.

…PATCH have a request body by default.

kaufholdr · 2017-02-28T16:26:49Z

Sure. I have added the required test cases. Are they sufficient?

gkalpak

We also need a test for POST/PUT/PATCH methods with hasBody: false.

gkalpak · 2017-03-01T11:28:20Z

src/ngResource/resource.js

@@ -193,6 +193,8 @@ function shallowClearAndCopy(src, dst) {
 *   - **`interceptor`** - `{Object=}` - The interceptor object has two optional methods -
 *     `response` and `responseError`. Both `response` and `responseError` interceptors get called
 *     with `http response` object. See {@link ng.$http $http interceptors}.
+ *   - **`hasBody`** - `{boolean}` - allows to specify if a request body should be included or not.
+ *     If not specified only POST, PUT and PATCH will have a request.


only POST, PUT and PATCH will have a request --> only POST, PUT and PATCH requests will have a body

gkalpak · 2017-03-01T11:30:58Z

test/ngResource/resourceSpec.js

@@ -97,6 +97,43 @@ describe('basic usage', function() {
    $httpBackend.flush();
  });

+  it('should include a request body when calling custom delete with hasBody is true', function() {


custom delete --> custom method

Can you also test with another method (in the same test), just to avoid giving the impression this is just about DELETE.

Extended test case with a custom CREATE request. Improved documentation of hasBody.

kaufholdr · 2017-03-06T10:38:27Z

I have added the requested changes.

Should I squash down my changes to one commit on completion or would you like to take them as several commits if it's ready?

gkalpak · 2017-03-06T12:53:04Z

test/ngResource/resourceSpec.js

+
+    var postResponse = R.post();
+    var putResponse = R.put();
+    var patchResponse = R.patch();


Would these 3 calls fail if hasBody was not set? I don't think so (but I haven't tried it either). Can you check?

No they don't fail if hasBody is not set. This is a bug or a feature of ngResource not enforcing the described api.

I haven't found a way to distinguish between request parameters and the request body in httpBackend.

Did I miss something?

If you pass two params (e.g. R.post(null, {foo: 'bar'})), then the first one is interpreted as params and the second one as data.

For example, the following test currently fails, because {id: -1} is not undefined. If something similar passes with your PR, then we are good:

$httpBackend.expectPOST('/foo').respond(function(method, url, data) { expect(data).not.toBeUndefined(); return [200, {id: 42}]; }); var R = $resource('/foo'); var response = R.save(null, {id: -1}); $httpBackend.flush(); expect(response.id).toBe(42);

sry for the delayed response on my side. I have merged your proposed test case into the test method with hasBody true. The only problem for me was that I couldn't reproduce the failing of the test case. (Even when I used your version.)

Can you see my error or give me the error that you get?

I was probably not clear. What I meant is that my test fails with the code currently in master (i.e. without your PR), which is expected, because we don't support the hasBody yet. If the test passes with your PR, then it verifies that the feature you added (hasBody) does indeed work for PUT/POST/PATCH requests too, so it is good to be merged.

gkalpak · 2017-03-06T12:56:32Z

test/ngResource/resourceSpec.js

+  it('should include a request body when calling custom method with hasBody is true', function() {
+    var instant = {name: 'info.txt', value: 'V2hlbiB0aGUgdGltZSBlbmRzLg=='};
+    var fid = 42;
+    var created = {fid: fid, filname: 'fooresource', value: 'V2hlbiB0aGUgdGltZSBlbmRzLg=='};


Nit: You don't really need these 2 extra variables (fid, created). You could inline them (and only keep the necessary props) to avoid visual noise.

I have cleaned up those variables.

gkalpak · 2017-03-06T12:59:02Z

You don't need to squash them, btw. We can take care of it while merging.

kaufholdr · 2017-03-10T15:46:23Z

Is there anything that I should fix in this Pull request right now?

gkalpak · 2017-03-12T12:56:28Z

@kaufholdr, that should be the last bit 😃

gkalpak · 2017-03-20T17:45:35Z

test/ngResource/resourceSpec.js

@@ -102,6 +102,10 @@ describe('basic usage', function() {
    var condition = {at: '2038-01-19 03:14:08'};

    $httpBackend.expect('CREATE', '/fooresource', instant).respond({fid: 42});
+    $httpBackend.expectPOST('/fooresource').respond(function(method, url, data) {


This is not needed here. POST is not a custom method (which what this test is about).

gkalpak · 2017-03-20T17:48:21Z

test/ngResource/resourceSpec.js

    expect(deleteResponse.$resolved).toBe(true);
  });

  it('should not include a request body if hasBody is false on POST, PUT and PATCH', function() {
-    $httpBackend.expect('POST', '/foo').respond({});
+    $httpBackend.expectPOST('/foo').respond(function(method, url, data) {


Make this change to all three methods. E.g. something like:

function verifyRequest(method, url , data) { expect(data).toBeUndefined(); return [200, {id: 42}]; } $httpBackend.expect('POST', '/foo').respond(verifyRequest); $httpBackend.expect('PUT', '/foo').respond(verifyRequest); $httpBackend.expect('PATCH', '/foo').respond(verifyRequest); ...

kaufholdr · 2017-03-20T20:37:19Z

Fell free to tell me if there is still something missing. :-)

gkalpak · 2017-03-21T08:18:43Z

test/ngResource/resourceSpec.js

+
+    expect(postResponse.id).toBe(42);
+    expect(putResponse.$resolved).toBe(true);
+    expect(patchResponse.$resolved).toBe(true);


Please, change these two lines to ....id).toBe(42).

… of $resolved.

kaufholdr · 2017-03-21T10:18:18Z

Thank. You. :-)

gkalpak · 2017-03-21T10:47:14Z

I tweaked the docs a little bit and merged. Thank you for working on this, @kaufholdr 👍

By default, only `PUT`, `POST` and `PATCH` requests have a body, but you can use `hasBody` to configure any action to either have or not have a body, regardless of its HTTP method. Fixes #10128 Closes #12181

googlebot added the cla: yes label Jun 21, 2015

yathos mentioned this pull request Jun 21, 2015

Add hasBody to ngResource action configuration #12180

Closed

Narretz added type: feature component: ngResource frequency: low labels Aug 8, 2015

Narretz added this to the Ice Box milestone Aug 8, 2015

Narretz force-pushed the master branch from 70049ae to b77e14b Compare January 16, 2016 21:59

Added hasBody to ngResource action configuration

745d347

kaufholdr force-pushed the master branch from 5723850 to 745d347 Compare December 5, 2016 12:37

gkalpak modified the milestones: 1.6.x (post 1.6.0), Ice Box Dec 5, 2016

gkalpak reviewed Dec 5, 2016

View reviewed changes

googlebot added cla: no and removed cla: yes labels Dec 6, 2016

Refined version of hasBody check.

c6f2687

kaufholdr force-pushed the master branch from a82516a to c6f2687 Compare December 6, 2016 10:00

Fixed documentation of ngResource.hasBody configuration.

9e23624

gkalpak added the needs: test label Feb 21, 2017

gkalpak suggested changes Feb 21, 2017

View reviewed changes

Improved documentation of ngResource.hasBody that only POST, PUT and …

be8aeeb

…PATCH have a request body by default.

kaufholdr force-pushed the master branch from d7ccabb to be8aeeb Compare February 28, 2017 09:20

Added unit test for ngResource.hasBody.

c8212ad

kaufholdr force-pushed the master branch from 9124870 to c8212ad Compare February 28, 2017 15:14

gkalpak suggested changes Mar 1, 2017

View reviewed changes

Added additional test case for hasBody: false on POST,PUT and PATCH

cf811f5

Extended test case with a custom CREATE request. Improved documentation of hasBody.

gkalpak reviewed Mar 6, 2017

View reviewed changes

Cleanup of hasBody test case variables.

36ce9f0

Enhanced test case on methods with hasBody true.

9139302

gkalpak suggested changes Mar 20, 2017

View reviewed changes

Further improvement of unit tests for ngResource hasBody.

77d9cfa

gkalpak suggested changes Mar 21, 2017

View reviewed changes

Changed expects of hasBody test to refere to a response value instead…

4e2a874

… of $resolved.

gkalpak approved these changes Mar 21, 2017

View reviewed changes

gkalpak closed this in d96e58f Mar 21, 2017

Add hasBody to ngResource action configuration #12181

Add hasBody to ngResource action configuration #12181

Conversation

kaufholdr commented Jun 21, 2015

caitp commented Jun 21, 2015

kaufholdr commented Jun 21, 2015

caitp commented Jun 21, 2015

kaufholdr commented Jun 21, 2015

yathos commented Jul 8, 2015

kaufholdr commented Jul 30, 2015

Narretz commented Aug 8, 2015

ElayneB commented Oct 26, 2016

kaufholdr commented Dec 5, 2016

gkalpak commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

kaufholdr Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Dec 6, 2016

googlebot commented Dec 6, 2016

googlebot commented Dec 6, 2016

googlebot commented Dec 6, 2016

kaufholdr commented Feb 20, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Feb 21, 2017

kaufholdr commented Feb 28, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaufholdr commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Mar 6, 2017

kaufholdr commented Mar 10, 2017

gkalpak commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaufholdr commented Mar 20, 2017

Choose a reason for hiding this comment

kaufholdr commented Mar 21, 2017

gkalpak commented Mar 21, 2017

gkalpak Dec 6, 2016 •

edited

Loading

kaufholdr Dec 6, 2016 •

edited

Loading

gkalpak Mar 12, 2017 •

edited

Loading