Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Don't copy server response to resource value if they are already the same object #4508

Closed

Conversation

jamesandersen
Copy link

EXPLANATION

An $http response interceptor may be used to compare the value sent to the server--available via response.config.data--to the value returned from the server--available via response.data--BEFORE logic inside $resource copies response.data to the resource instance value. The logic of this response interceptor MAY, in some cases (see use case below), set the value of response.data to response.config.data which, when using $resource also happens to be a resource instance value.

This very simple pull request simply skips $resource's attempt to copy the response.data to the resource value if the two already reference the same object. Without this change, the copy() function will throw when called from within the resource.

USE CASE

The use case leading to this pull request is as follows. In an application implementing auto-save, $resource is used to periodically persist in-progress changes (identified using FormController.$dirty) to the server. While that save request is outstanding, the user may continue to make changes. Because $resource's default logic will discard any changes made between the time the save request was issued and it returns using copy(data, value);, a response interceptor has been implemented with custom application logic to merge three states of the object being saved:

  1. at the time request was issued
  2. the object's current state (possibly modified by the user since request was issued)
  3. the object state returned from the server (wherein the server may have made changes e.g. optimistic concurrency, etc.)

The application's custom merge logic, to avoid firing unnecessary watches or losing the user's changes, merges states (1) and (3) onto the existing object which is response.config.data and then sets response.data to response.config.data. This currently causes $resource to throw and requires the very minor customization in this pull request. I'd LOVE to NOT have a customized copy of ngResource in my app.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@petebacondarwin
Copy link
Contributor

I am wondering what the reason behind throwing an error in copy() when src and dest are identical. It would seem easier for it to be a noop. @IgorMinar ?
It was introduced by this commit: 08029c7

@ghost ghost assigned IgorMinar Nov 2, 2013
@IgorMinar
Copy link
Contributor

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@jamesandersen
Copy link
Author

Hmm... thought I had done that previously but I just completed the CLA again.

@petebacondarwin
Copy link
Contributor

I just checked and the test in this PR already passes on master. This is because we are now using the shallowClearAndCopy utility instead of a straight copy.

@jamesandersen jamesandersen deleted the dont-copy-resource branch June 6, 2016 18:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants