feat: Allow saving with custom objectId

[dplewis]

Closes: #1097
Ref: parse-community/parse-server#6177

@JeromeDeLeon

[codecov]

Codecov Report

Merging #1309 (ef55fb4) into master (801dcd6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1309   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           60        60           
  Lines         5836      5851   +15     
  Branches      1310      1314    +4     
=========================================
+ Hits          5836      5851   +15     

Impacted Files	Coverage Δ
src/CoreManager.js	100.00% <ø> (ø)
src/Parse.js	100.00% <100.00%> (ø)
src/ParseObject.js	100.00% <100.00%> (ø)

---

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 801dcd6...ef55fb4. Read the comment docs.

[mtrezza]

LGTM!

[JeromeDeLeon]

LGTM! Just a question, if we were to merge this and update the Parse-Server, would it be allowed to use with save trigger?

[dplewis]

@JeromeDeLeon The purpose of this PR is to allow the customId to get sent to the server. I wrote a quick test and the beforeSave trigger gets called but the behavior looks wrong. Might need a server update.

[mstniy]

This forces all objects to have custom ids, if custom ids are allowed.
Is this intentional? This is not how the server treats allowCustomObjectId.

[bdevore17]

I second this. Just turned this on, and this does not work as expected.

[cbaker6]

I recommend looking at #1309 (comment)

[breitman]

Hi, I am trying to set my own Id with this new feature. In my parse options for parse-server, I added the allowCustomObjectId to be true. Is there another thing i need to do with Parse JS SDK?

[mstniy]

Currently, one also needs to set allowCustomObjectId on the client side, which I find to be weird. It is used to decide between doing a POST and a PUT, in conjunction with updatedAt, which I find to be even weirder. Perhaps a later PR can modify the js sdk to have a create()/update() seperation instead, like the Dart sdk?

[mtrezza]

Currently, one also needs to set allowCustomObjectId on the client side, which I find to be weird. It is used to decide between doing a POST and a PUT, in conjunction with updatedAt, which I find to be even weirder.

That indeed needs a closer look. Can you give more details / examples for what you described?

The difference between POST and PUT should whether the object already exists or is newly created, not how the object ID is set.

[breitman]

Currently, one also needs to set allowCustomObjectId on the client side, which I find to be weird. It is used to decide between doing a POST and a PUT, in conjunction with updatedAt, which I find to be even weirder. Perhaps a later PR can modify the js sdk to have a create()/update() seperation instead, like the Dart sdk?

So what is the notation to allow for custom IDs on the client side? Parse-server automatically creates the js sdk so i am confused as where i declare it on client side

[mstniy]

Parse.allowCustomObjectId = true; , as you can also see in the unit tests added by this PR. It can go anywhere, but it must be set by the time you call .save()
Another quirk is that this flag forces all objects to have a custom id, so it is really a forceCustomObjectId . I would love for this to change. In the meanwhile, do not forget to set it back to false after the operation involving custom object ids.

I am not sure what you mean by the server automatically creating the sdk.

[breitman]

Parse.allowCustomObjectId = true; , as you can also see in the unit tests added by this PR. It can go anywhere, but it must be set by the time you call .save()
Another quirk is that this flag forces all objects to have a custom id, so it is really a forceCustomObjectId . I would love for this to change. In the meanwhile, do not forget to set it back to false after the operation involving custom object ids.

I am not sure what you mean by the server automatically creating the sdk.

That isn't working. I added that line to the top of my cloud function, and i am still getting an object not found error, due to the custom id....

[mstniy]

Weird. Unless you are using an outdated version of the server, you might want to open an issue.
I can see the latest version on npm is 4.5.0, which was released before this PR was merged.

[mtrezza]

In the meanwhile, do not forget to set it back to false after the operation involving custom object ids.

Are you intending to switch between custom and non-custom IDs during runtime? I'm not sure such a fundamental change should be possible during run-time, or even at all for a deployment.

[mstniy]

Currently, one also needs to set allowCustomObjectId on the client side, which I find to be weird. It is used to decide between doing a POST and a PUT, in conjunction with updatedAt, which I find to be even weirder.

That indeed needs a closer look. Can you give more details / examples for what you described?

The difference between POST and PUT should whether the object already exists or is newly created, not how the object ID is set.

Please see the code. Because the server doesn't support upsert (see parse-community/parse-server#7139), the SDK ends up having to guess whether to use POST (which fails if the object already exists) or PUT (which fails if the object does not already exist). The way it does that is by checking if createdAt is set.

To see the way the Flutter SDK does it, see its source. It has a .create() that always does a POST, an.update() that always does a PUT, as well as a .save() that tries to guess.

[mstniy]

In the meanwhile, do not forget to set it back to false after the operation involving custom object ids.

Are you intending to switch between custom and non-custom IDs during runtime? I'm not sure such a fundamental change should be possible during run-time, or even at all for a deployment.

We want to allow custom ids, but the changes in this PR forces us to either always use custom object ids, or to disallow them altogether. The way we currently work around this is by toggling the sdk's flag. The server is set to allow custom object ids all the time.

[lsmilek1]

In my application I found also a use case for the mixed environment of server generated and custom objectId (ParseSwift SDK, forum) and would like to implement allow instead of force.

From what I tested with forked ParseSwift SDK where I removed the guards that are checking that objectId is always set when allowCustomObjectId = true on both the server and the client, it works as intended:

• creating object with server generated objectId when ParseObject.objectId = nil and ParseObject.createdAt = nil
• creating object with custom objectId when ParseObject.objectId = customId and ParseObject.createdAt = nil
• updating object with any objectId when ParseObject.objectId = any id and ParseObject.createdAt = any date
• it throws error "Duplicate value" when trying to create object with custom id that already exists

So from this point of view there is no urgent need for separate .create() and .update() as long as it would be clearly documented that the createdAt is needed to decide whether the object should be updated or created - at least from client SDK, I am not 100% sure thing could break on server side JS SDK. But again, as it is already working and together with the documented need of createdAt, is there something blocking this to be implemented? In my use case if would safe few fields and indexes in the database together with extra fetches in the cloud code triggers.

[cbaker6]

This forces all objects to have custom ids, if custom ids are allowed.
Is this intentional? This is not how the server treats allowCustomObjectId.

Though what @mstniy stated above is true, particularly "forces all objects to have custom ids..." . I don't think removing the checks that @dplewis added should be done as they make sure the proper items are needed when using a custom objectId. Instead, I recommend using an approach similar to the Swift SDK parse-community/Parse-Swift#222 (comment) in which a flag, isIgnoreCustomObjectIdConfig is added to .save and if the developer wants to ignore the check to use a mixed environment, they can. The reasons for this are in the warnings added to the documentation:

If you are using ParseConfiguration.allowCustomObjectId = true and plan to generate all of your objectId's on the client-side then you should leave isIgnoreCustomObjectIdConfig = false. Setting ParseConfiguration.allowCustomObjectId = true and isIgnoreCustomObjectIdConfig = true means the client will generate objectId's and the server will generate an objectId only when the client does not provide one. This can increase the probability of colliding objectId's as the client and server objectId's may be generated using different algorithms. This can also lead to overwriting of ParseObject's by accident as the client-side checks are disabled. Developers are responsible for handling such cases.`

Basically, if the developer "really" wants a mixed environment, they should do so at their own risk. I can easily see devs raising issues on the server and SDK repos for handling the above warning incorrectly.

[lsmilek1]

I see, that could be also a way to go. I am just still missing what is being protected exactly or at least it feels redundant, because from what I tested in ParseSwift SDK, it behaves correctly (assuming the allowCustomObjectId = true has to be set on the server). To summarise the behaviour I have in mind:

1. allowCustomObjectId = false

• objectId = nil --> save new object
• objectId = generatedId --> update object with objectId == generatedId or throw "does not exist"

2. allowCustomObjectId = true

• objectId = nil, createdAt = nil --> save new object with generated id
• objectId = customId, createdAt = some date --> save new object with custom id
• objectId = someId, createdAt = some date --> update object with objectId == someId or throw "does not exist"
• objectId = someId, createdAt = nil --> throw "duplicate value"

At least with ParseSwift SDK it work that way when I turned of the guards in forked SDK.

Edit: in your comit I can see the concern and finally understood it!