Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ParseObject): Add option cascadeSave to save() #881

Merged

Conversation

RaschidJFR
Copy link
Contributor

The new feature can be used to prevent cascade saving in nested objects (Only ignores objects that have already been saved previously):

parentObject.save(null, { cascadeSave: false });  // Child objects will not be saved (again)

Closes #864

@davimacedo
Copy link
Member

@RaschidJFR thanks for the PR. Can you please take a look in the failing tests?

@RaschidJFR
Copy link
Contributor Author

I'm checking it. It seems it could easily be fixed by adding a safe condition here, but it seems strange to me as this line should always return a defined array. So what do you think @davimacedo , should I just do:

let unsaved = unsavedChildren(this);  // <- This is supposed to always return a defined array
unsaved = unsaved && unsaved.filter(o => o._localId || options.cascadeSave !== false) || []

@davimacedo
Copy link
Member

I'd do something like this:

    let cascadePromise = Promise.resolve();
    if (options.cascadeSave !== false) {
      cascadePromise = controller.save(unsavedChildren(this), saveOptions);
    }
    return cascadePromise.then(() => {
      return controller.save(this, saveOptions);
    });

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #881 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #881   +/-   ##
======================================
  Coverage    92.1%   92.1%           
======================================
  Files          54      54           
  Lines        5026    5026           
  Branches     1126    1127    +1     
======================================
  Hits         4629    4629           
  Misses        397     397
Impacted Files Coverage Δ
src/ParseObject.js 90.21% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4023bdd...b479793. Read the comment docs.

@RaschidJFR
Copy link
Contributor Author

I'd do something like this:

    let cascadePromise = Promise.resolve();
    if (options.cascadeSave !== false) {
      cascadePromise = controller.save(unsavedChildren(this), saveOptions);
    }
    return cascadePromise.then(() => {
      return controller.save(this, saveOptions);
    });

Thanks for the suggestion @davimacedo , but if I did that new objects wouldn't be saved at all with this option. So I just added the safe check. It seems to work know. Let me know.

@davimacedo
Copy link
Member

hum... so your idea is, when cascadeSave set to false, not save updates anymore but still save the creates? It is very confusing in my opinion.

@RaschidJFR
Copy link
Contributor Author

Well yes. I think new objects must always be saved, otherwise you may run into unexpected broken binds later on. The idea here is to prevent Parse from saving objects you do not explicitly intend to save, but keeping relational data working.

Maybe it could become less confusing if we try different more explicit name for the parameter?

@davimacedo
Copy link
Member

I don't know. In my opinion it should either save everything (if cascade set to true, which is the default) or save only the object which save() method is invoked (is cascade set to false). Having other objects been saved with cascade set to false is weird in my opinion. For the relationships you can still call the save() method for each of the pointers.

For example:

const a1 = new Parse.Object('A');
await a1.save({ cascade: false });
const a2 = new Parse.Object('A');
a1.set('somefield', 'somevalue');
a2.set('somefield', 'somevalue');
const b = new Parse.Object('B');
b1.set('pointer1', a1);
b1.set('pointer2', a2);
await b1.save({ cascade: false });

Now a1 has somefield set to undefined and a2 has somefield set to somevalue. That's really a strange behavior for me. For this case, in my opinion, ideally, the developer should receive an error "a2 should be saved before used as a pointer".

Nested objects who have not been saved will be ignored when setting `cascadeSave: true`, so save() will throw "Cannot create Pointer to an unsaved ParseObject".
@RaschidJFR
Copy link
Contributor Author

It makes sense. It should be enough throwing the default error then.

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. @acinader @dplewis any thoughts?

@davimacedo davimacedo requested review from dplewis and acinader August 7, 2019 05:58
@davimacedo davimacedo merged commit c061e2f into parse-community:master Aug 7, 2019
@davimacedo
Copy link
Member

@RaschidJFR thanks for the contribution!!! Would you be willed to also add some note about this new option in JS Docs?

@RaschidJFR
Copy link
Contributor Author

Sure. I'm already working on it. What about the api reference? is that automatically generated from the code here?

@davimacedo
Copy link
Member

Nice! The API reference is already updated: https://parseplatform.org/Parse-SDK-JS/api/master/Parse.Object.html#save :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to prevent cascade saving
3 participants