setPlainByDefault does not strip etag from responses

[lucioperca]

Restangular is configured with setPlainByDefault(true). When running a query such as get() or getList() that retrieves a response with an ETag exposed, the result contains restangularEtag.

angular.module('app', ['restangular'])
  .config(function(RestangularProvider) {
    RestangularProvider.setBaseUrl('https://exposedetagresponder.org/');
    RestangularProvider.setPlainByDefault(true);
  })
  .controller('main', function($scope, Restangular) {
    Restangular.one('resource').get().then(elem => {
      console.log(elem.restangularEtag); // Outputs the response's etag
    });
    Restangular.all('resources').getList().then(list => {
      console.log(list.restangularEtag); // Outputs the response's etag
    });
  });

This is inconsistent with the behavior when calling plain() on the result directly, even though these are apparently intended to have the same outcome. (94ffaf0)

angular.module('app', ['restangular'])
  .config(function(RestangularProvider) {
    RestangularProvider.setBaseUrl('https://exposedetagresponder.org/');
  })
  .controller('main', function($scope, Restangular) {
    Restangular.one('resource').get().then(elem => {
      console.log(elem.plain().restangularEtag); // Outputs undefined
    });
    Restangular.all('resources').getList().then(list => {
      console.log(list.plain().restangularEtag); // Outputs undefined
    });
  });

[bostrom]

@lucioperca I added some unit tests for your use case and they come through clean without modification of Restangular.

Can you check the PR and see if the tests makes sense, especially here? I'm doing a GET operation which responds with a header ETag with an UUID as value. When setPlainByDefault is set to true, the resulting object does not contain the restangularEtag field.

What version of Restangular are you using?

[lucioperca]

@bostrom I checked the tests in the PR. It looks like the whenGET cases in the new tests for setPlainByDefault may be using the responses designed in the top level beforeEach call, rather than the ones defined for that specific test case. You can see a variation on the tests here, which is failing 'correctly':

#1457

[lucioperca]

This seems to happen because parseResponse populates the ETag data, which is called before the config.plainByDefault check, whereas the rest of the restangularized methods are set in restangularizeElem / restangularizeCollection, which is called after the check.

https://github.com/mgonto/restangular/blob/master/src/restangular.js#L1140

[lucioperca]

Took a stab at fixing this (as well as fixing one of my modified unit tests). See updated:

#1457

[bostrom]

Yes, you're right, my whenGET didn't override the default response for the request, how stupid.

Thanks for the PR, awesome! I see the problem now, it's the fact that the restangularEtag field is set in the parseResponse function which is run before the "unrestangularized" element is passed to the caller. In normal cases all Restangular fields are set in restangularizeElement which is called after the "resolve-if-plainByDefault".

EDIT: Actually that's just what you said 🙄

This seems to happen because parseResponse populates the ETag data, which is called before the config.plainByDefault check, whereas the rest of the restangularized methods are set in restangularizeElem / restangularizeCollection, which is called after the check.

I would consider that the bug here, and moving the setting of restangularEtag into restangularizeElem would be a better solution IMO. Deleting seemingly "random" properties at arbitrary points in time makes it a bit harder to follow the general workflow (which at the moment is a mess anyway, but let's not make it worse).

What do you think, can you update your PR and instead of deleting the etag property, you would set it inside the restangularizeElem function where it makes more sense?

[lucioperca]

@bostrom I agree with that idea in principle, but I'm not sure how best to go about it. It looks like restangularizeElem and restangularizeCollection look like they don't receive any other parameters that contain the response ETag. parseResponse has it because it receives the basic response as an input (https://github.com/mgonto/restangular/blob/master/src/restangular.js#L1110) but the restangularize functions only get the response data as parsed by parseResponse. I could add some other parameter to restangularizeElem (etag? response?), but this would modify the signature of the exposed restangularizeElement function.

If I can find time I'll fiddle around with it and see if I can modify the function given the above.

[bostrom]

@lucioperca yes, I noticed that it might require some more profound changes due to the facts you present. But have a look what you can do if you have time, and if not then I'll return to this issue in a while.

I'm just now in the process of refactoring the codebase a bit to be able to write some better unit tests. With the unit tests in place, I'll hopefully be able to restructure the code to cater for easier bug fixes and features. It's a tedious and time consuming process though... 😓

[Malex]

Any workaround that you know of to solve this issue? It's really frustrating I can't seem to be able to handle this

[Malex]

Maybe I am just dumb, but why cant perseResponse be moved after the plain return?