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

Save pointer to new object in beforeSave #5186

Closed
mtrezza opened this issue Nov 20, 2018 · 28 comments
Closed

Save pointer to new object in beforeSave #5186

mtrezza opened this issue Nov 20, 2018 · 28 comments

Comments

@mtrezza
Copy link
Member

mtrezza commented Nov 20, 2018

Issue Description

It is currently not possible to reference an object in its beforeSave hook to create a new object with a pointer to it.

Steps to reproduce

Example scenario:
A user in User class should have a user profile in UserProfile class associated with it. A user profile has a pointer to the user.

It is currently only possible to create the user profile in the user's afterSave hook. But immediately after saving the user, other API calls may look for the user profile which does not exist. It is not a solution to create a user profile if it does not exists, as that can cause race conditions in which multiple user profiles are created. It is also not a solution to set the pointer to the user as unique in the mongoDB collection which prevents multiple user profile from being created but also causes data loss, because the information to update the user profile with is lost after the failed call.

Expected Results

It should be possible to create a pointer to an object in its beforeSave hook.

Actual Outcome

Trying to save a user profile causes a timeout in path="/parse/batch".

Environment Setup

  • Server

    • parse-server version: 2.8.4
    • Localhost or remote server: Heroku
  • Database

    • MongoDB version: 3.6.7
    • Localhost or remote server: mLab

Logs/Trace

@flovilmart
Copy link
Contributor

This is the expected behavior as the object is not stored in the DB yet and doesn’t have an objectid that is stored.

Why can’t you use the afterSave for this intent?

@mtrezza
Copy link
Member Author

mtrezza commented Nov 20, 2018 via email

@flovilmart
Copy link
Contributor

ok; I believe I get the problem, however creating a pointer to an unexisting object is not allowed, and at the time of the save, the object is not saved (by definition). Anything could prevent the object from saving actually.

What do you suggest?

@acinader
Copy link
Contributor

@mtrezza, you had the right idea asking this on StackOverflow: https://stackoverflow.com/questions/53321248/how-to-prevent-race-condition-when-creating-a-document-on-parse-server

this is a design issue that you can solve.

I'm glad to continue helping you on that thread.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 20, 2018

@acinader, thank you, any help appreciated.

My understanding is that there is no solution to the issue. There is always a possible race consition. Hence the issue opened here to reflect on the parse server design.

What is your suggestion to solve this?

@acinader
Copy link
Contributor

I asked you a question on SO: "so why even bother doing #2? why not count on #4 to do the work."

Danh and I are trying to help you, but I am currently lacking enough information to do that.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 20, 2018

@acinader I already answered this on SO two days ago. Which information specifically do you need?

Again, there seems to be no way to avoid a race condition so I think it is not just a debugging issue.

@flovilmart
Copy link
Contributor

While I appreciate all kinds of discussions, this one has nothing to do here @mtrezza =. double posting on multiple forums is never the way to go. Here, I particularly addressed the fact that what you are facing is indeed not a bug. If you wish to open a PR and suggest a 'safe' way to save pointers to unexisting objects feel free to do so. Otherwise, I would appreciate you stick with stack overflow.
Never you mentioned in your description you'd open a question outside. Not being satisfied with the answers on stackoverflow this is not a valid answer. And yes race condition there is. If you want to avoid races, I would recommend you use atomic updates, or another way to structure your data.. OR use another DB technology like Postgres that offer transactions.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 20, 2018

@flovilmart Agreed. As the community confirms that parse server cannot do this by design, I will look into opening a PR. I just wanted to make sure that I did not miss a feature or even an existing PR that actually addresses this.

@acinader
Copy link
Contributor

In this case, since you're not trying to coordinate multiple clients, I think that there are ways to work around the problem with parse server as is. But, there are definitely times when I could have used a 'blocking after-save hook' where the object is available in its saved state before the client gets a response. Glad to re-open @mtrezza if you want to work through it.

@acinader acinader reopened this Nov 21, 2018
@flovilmart
Copy link
Contributor

I’ll be closing again as stalled and against the basic protection that the pointer objects should exist in the DB.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

OK, I will look into a PR when I have more time.

If anyone interested, the currently used workaround:

  • a unique index in mongoDB on the user field in UserProfile collection
  • a Parse.Cloud function that returns the user profile and creates one if none exists yet

To address concurrency: if no user profile was found and creating a user profile fails with Parse error 137 DuplicateValue, then the function queries again for an existing user profile.

@flovilmart
Copy link
Contributor

@mtrezza what do you consider as a PR? Because I don't want you to start working on a PR if we know ahead of time that we'll reject it.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

@flovilmart The PR should make it possible to create another document in a collection inside a beforeSafe hook which contains a reference the the document for which the beforeSafe hook was called.

For example:

  1. Create user in User collection
  2. beforeSafe hook triggered for User collection
  3. Within beforeSafe, create a userProfile in UserProfile that references the User that is about to be created
  4. Completion of beforeSafe:
    4a. On success: Create the user and its userProfile with a pointer to the user
    4b. On failure: Don't create user or userProfile

However, before working on a PR, it's a good to re-start discussion here.

@flovilmart
Copy link
Contributor

Let me challenge that with the following example:

Parse.Cloud.beforeSave(Parse.User, async (req) => { 
  const user = req.object;
  const profile = new Parse.Object('UserProfile');
  profile.set({ user });
  await profile.save();

  throw new Error('There was a proble saving the user');
});

In the following example, there may be an issue after the profile saves the user's reference. So in this case you'll have a dangling reference on the profile which is not a good idea either.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

I think this would be the current implementation of Parse Server. The PR should address exactly the issue you mention, where only when saving the user succeeds, the user profile would be created with a user reference. The PR would add that logic.

For example if we introduced a profile.saveOnSuccess(), then the profile would only be saved if the user was saved successfully.

@flovilmart
Copy link
Contributor

No, what I mean is that if there is an issue after you attach the user to the userProfile, (denoted with the throw) then, you profile has a pointer, but it points to nowhere. You still end up creating an object for nothing.

If you change your logic to have the user have a userProfile then there is no issue. Because you want a cyclic relation user <-> profile, I would recommend to revise your data structure. You can very well attach the profile to the user and in the afterSave, do the reverse attach the profile to the user

@flovilmart
Copy link
Contributor

flovilmart commented Dec 30, 2018

Parse.Cloud.beforeSave(Parse.User, (req) => {
  const user = req.object;
  if (!user.get('profile') && user.isNew()) {
    const profile = new Parse.Object('UserProfile');
    await profile.save();
    user.set({ profile });
  } 
});

Parse.Cloud.afterSave(Parse.User, (req) => {
  const user = req.object;
  // In afterSave, you need to use this equality to detect the 1st save
  // user.isNew() is only valid if the objectId is not set
  if (user.createdAt === user.updatedAt) {
    user.get('profile').set({ user });
    // uncomment return if you want to wait till the profile saved
    /* return */ user.get('profile').save();
  } 
});

What is wrong with this approach? In any case, because you associate the profile with the user save, you will need to fetch in from the client. The server is designed to return any mutations that occur in the before/after hooks.

Also, with version > 3 you can wait till the afterSave has completed.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

This approach causes the profile to be without user reference before afterSave has completed. So a profile can only be safely accessed through the User class with an include statement.

The conclusion would be to not add a user reference to the profile at all, because it is not guaranteed to exist . But without reference, the UserProfile class cannot be queried meaningfully, because it is missing the user reference. On the other hand the User class does not support query parameters with dot notation for the profile.

So the requirement is that the profile needs a user reference from the moment it is created.

@flovilmart
Copy link
Contributor

well, just trying to give your the proper way of doing it, without breaking anything and with enough guarantees that the profile saves occurs within the user save cycle. If you manage to query a profile in between this cycle, then I’m not sure what to tell you, but again, consider transactions instead. Whatever you are attempting is also wrong as you would be saving the profile to an unexisting user.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

Going back to my suggestion, what speaks against introducing a profile.saveOnSuccess() in the beforeSave hook?

Behind the curtain, when the beforeSave hook saved the user, but before it calls the completion handler, it could save the user profile with the created user as reference. And it's not a breaking change.

Parse.Cloud.beforeSave(Parse.User, (req) => {
  const user = req.object;
  if (!user.get('profile') && user.isNew()) {
    const profile = new Parse.Object('UserProfile');
    profile.set("user", user);
    profile.saveOnSuccess();
  } 
});

The beforeSave hook executes behind the curtain:

  1. Save user
  2. Save profile (with proper user reference and only if saving user succeeded)
  3. Call completion handler

@flovilmart
Copy link
Contributor

what speaks against introducing a profile.safeOnSuccess() in the beforeSave hook?

This is a quite bad solution, If you want to run something after the object is saved then use afterSave.

Did you even try the solution that I recommend, that work and is reliable?

After that, what you want / describe, is a transaction. That exactly many operations occur or none. This is an interesting take on transactions, to be able to trigger them from a beforeSave, and add operations to the 'current' transaction, reusing the result of the previous operation (like the objectId saved).

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

Did you even try the solution that I recommend, that work and is reliable?

I tried it with beforeSave and afterSave hook, but it didn't work for me. The client creates a user with signupInBackground and immediately after sign-up returns, calls a cloud function to update fields in the profile. The cloud function queries for a profile but doesn't find one, because the profile does not contain a user reference yet. The afterSave hook did not complete yet. That was the original issue.

Using mongoDB, I am unsure how to use transactions to solve this.

@flovilmart
Copy link
Contributor

The afterSave hook did not complete yet.

Which has been solved for a while as if you return a promise in the afterSave hook, this will wait for the promise to complete before responding to the user.

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

But the signUpInBackground completion handler in iOS SDK will be called before afterSave.

iOS:

// Create user
let user = PFUser()
user.username = username
user.password = password

// Sign up user
user.signUpInBackground() { (succeeded, error) in 

    // Update profile
    PFCloud.callFunction(inBackground: "updateProfile", withParameters: [...]) { (response, error) in

        // Fails; cloud function cannot find a profile with the user reference because afterSave hook has not completed.
    }
}

Cloud Code:

Parse.Cloud.define("updateProfile", function(request, response) {

    // Find user profile
    let query = new Parse.Query("Profile");
    query.equalTo("user", request.user);
    ...
}

@flovilmart
Copy link
Contributor

No it won’t if you return a promise in your afterSave hook

@mtrezza
Copy link
Member Author

mtrezza commented Dec 30, 2018

@flovilmart That actually solves the issue and makes the PR unnecessary. Took me a while to understand your example, thanks so much for your patience.

Maybe interesting for @acinader who wrote:

there are definitely times when I could have used a 'blocking after-save hook' where the object is available in its saved state before the client gets a response.

@flovilmart
Copy link
Contributor

@mtrezza good! However there is still a slight chance that the _User object save fails, and an dangling profile is created, which is not a big deal, as they are easily identifiable as they lack the back reference to the user (but you can always destroy them in batch as they should not be referenced by anyone).

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

No branches or pull requests

3 participants