Open lucioperca opened 7 years ago
@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?
@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':
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
Took a stab at fixing this (as well as fixing one of my modified unit tests). See updated:
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 :roll_eyes:
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?
@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.
@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... 😓
Any workaround that you know of to solve this issue? It's really frustrating I can't seem to be able to handle this
Maybe I am just dumb, but why cant perseResponse be moved after the plain return?
Restangular is configured with
setPlainByDefault(true)
. When running a query such asget()
orgetList()
that retrieves a response with an ETag exposed, the result contains restangularEtag.This is inconsistent with the behavior when calling
plain()
on the result directly, even though these are apparently intended to have the same outcome. (https://github.com/mgonto/restangular/commit/94ffaf0b72a9c10711cabec2d41ceae10282162c)