Repository navigation
setPlainByDefault does not strip etag from responses #1453
Description
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
});
});
Activity
test(ETags): add tests for ETag headers and setPlainByDefault
bostrom commentedon Jan 14, 2017
@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
ETagwith an UUID as value. WhensetPlainByDefaultis set totrue, the resulting object does not contain therestangularEtagfield.What version of Restangular are you using?
lucioperca commentedon Jan 17, 2017
@bostrom I checked the tests in the PR. It looks like the
whenGETcases in the new tests forsetPlainByDefaultmay be using the responses designed in the top levelbeforeEachcall, 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 commentedon Jan 17, 2017
This seems to happen because
parseResponsepopulates the ETag data, which is called before theconfig.plainByDefaultcheck, whereas the rest of the restangularized methods are set inrestangularizeElem/restangularizeCollection, which is called after the check.https://github.com/mgonto/restangular/blob/master/src/restangular.js#L1140
lucioperca commentedon Jan 17, 2017
Took a stab at fixing this (as well as fixing one of my modified unit tests). See updated:
#1457
bostrom commentedon Jan 18, 2017
Yes, you're right, my
whenGETdidn'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
parseResponsefunction which is run before the "unrestangularized" element is passed to the caller. In normal cases all Restangular fields are set inrestangularizeElementwhich is called after the "resolve-if-plainByDefault".EDIT: Actually that's just what you said 🙄
I would consider that the bug here, and moving the setting of
restangularEtagintorestangularizeElemwould 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
restangularizeElemfunction where it makes more sense?lucioperca commentedon Jan 19, 2017
@bostrom I agree with that idea in principle, but I'm not sure how best to go about it. It looks like
restangularizeElemandrestangularizeCollectionlook like they don't receive any other parameters that contain the response ETag.parseResponsehas it because it receives the basic response as an input (https://github.com/mgonto/restangular/blob/master/src/restangular.js#L1110) but therestangularizefunctions only get the response data as parsed byparseResponse. I could add some other parameter torestangularizeElem(etag? response?), but this would modify the signature of the exposedrestangularizeElementfunction.If I can find time I'll fiddle around with it and see if I can modify the function given the above.
bostrom commentedon Jan 19, 2017
@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 commentedon Jan 30, 2019
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 commentedon Jan 30, 2019
Maybe I am just dumb, but why cant perseResponse be moved after the plain return?