Allow mixed environment of server generated and custom objectId

[lsmilek1]

New Feature / Enhancement Checklist

• I am not disclosing a vulnerability.
• I am not just asking a question.
• I have searched through existing issues.

Current Limitation

Currently it is not possible to have mixed environment of custom and server generated objectId. In the case where User and user's Profile are two different objects in the database (keeping public data only in Profile) there is a need of reference field between these two related objects (Pointer or objectId String).

This is making it complicated in certain scenarios. For example some third object, let it be Group has a list of participants [objectId] or Pointers. When the group gets deleted and the cloud code function would like to send a message to each remaining participant, it needs to fetch each Profile's data to get related User's objectId to be able to set the ACL of the "group deleted" message object only for that recipient.

Feature / Enhancement Description

With allowCustomObjectId = true allow mixed environment so that it is possible to set the same objectId for multiple objects in different classes. This allows User and Profile to have the same objectId and there will be no need to fetch additional objects to set the ACL properly as mentioned in the above scenario, reducing also the size of the objects in the database and need of additional indexes on reference field

Example Use Case

For simple test I removed the if check on saveCommand() and it works as intended, having the need of not-nil createdAt if the object should be updated.

lsmilek1@b3346df

Leaving createdAt = nil and setting custom objectId to already existing value behaves as intended:

error saving testInbox: ParseError code=137 error=A duplicate value for a field with unique values was provided

Alternatives / Workarounds

As mentioned, available workaround is adding extra fields and indexes to the objects and therefore additional load on the server at big scale. In example of User and Profile it is also easier to read that certain object belongs together

3rd Party References

[lsmilek1]

Do I miss any security or other reason of that if ParseSwift.configuration.allowCustomObjectId && objectId == nil { ... } checks? Because simply leaving them out would allow the behaviour that would noticeably simplify the schema I am using.

[cbaker6]

I don’t fully understand what’s being proposed. It seems there are multiple topics mentioned.

With allowCustomObjectId = true allow mixed environment so that it is possible to set the same objectId for multiple objects in different classes. This allows User and Profile to have the same objectId and there will be no need to fetch additional objects to set the ACL properly as mentioned in the above scenario, reducing also the size of the objects in the database and need of additional indexes on reference field

This defeats the purpose of having a unique objectId IMO as it’s no longer unique across tables and therefore no longer unique

Related discussions:

• feat: Allow saving with custom objectId Parse-SDK-JS#1309 (comment)
• Allow to set createdAt and updatedAt during data import(s) parse-server#7447
• https://community.parseplatform.org/t/both-custom-and-generated-objectid-in-one-project/1812/2?u=cbaker6

[cbaker6]

A PR to allow a mixed environment should probably follow the ideas mentioned in parse-community/Parse-SDK-JS#1309 (comment)

Mainly, adapting components of the Flutter SDK. Thinking about this from a high level, you would leave the saveCommand the way it currently is and use save to guess to PUT/POST based on the options enabled. Then add a .create() and .update() to ParseObject, ParseUser, and ParseInstallation that call the create and update commands respectively. You will need to make the synchronous and asynchronous versions using the current .save() in the SDK as an example. You will also need to add the respective testcases.

If this is something you would like to try, feel free to open a PR

[lsmilek1]

As we already exchanged some discussion regarding this in past and I went through the links I believe this does not need to be very complicated. In principle, the server already does support the mixed environment and it decides based on createdAt whether to save or create object. ParseSwift SDK is already checking the createdAt in the conjunction with allowCustomObjectId. I believe that not deciding based on objectId in case of allowCustomObjectId = true would be aligned with the server logic and what you mentioned in the last comment:

     /// Specifies if a `ParseObject` has been saved. 
     public var isSaved: Bool { 
         if !ParseSwift.configuration.allowCustomObjectId { 
             return objectId != nil 
         } else { 
             return /*objectId != nil &&*/ createdAt != nil 
         } 
     }

Unless I am missing anything the behaviour of server and SDK would stay the same. The SDK's users have already now to set createdAt to any date if they want to update the object. On the other hand if they want to create a new object and therefore they keep createdAt = nil the server and SDK show proper error already that the object is already existing if existing objectId is being set:

error saving testInbox: ParseError code=137 error=A duplicate value for a field with unique values was provided

And for allowCustomObjectId = false this would not change any behaviour. As both of the modification do not affect the code path in that regard - only checks for allowCustomObjectId = true are modified:

if ParseSwift.configuration.allowCustomObjectId && object.objectId == nil { ... }

And that does not seems to interfere with any test cases and linked discussion as far I understand things right.

[cbaker6]

In principle, the server already does support the mixed environment and it decides based on createdAt whether to save or create object.

Where do you see this at?

And that does not seems to interfere with any test cases and linked discussion as far I understand things right.

This will have to be shown through the CI and even if none of the tests fail, it doesn't mean something isn't missed.

[lsmilek1]

You and Manuel mentioned it here:
parse-community/parse-server#7447 (comment)

with reference to JS SDK , where I understand that the JS SDK also do not check if objectId exists when allowCustomObjectId = true, so what I am proposing is to align this SDK with the JS SDK basically:
https://github.com/parse-community/Parse-SDK-JS/blob/ffc523f9cd0fa2dd3fcc9754d5f15c3c1c78314f/src/ParseObject.js#L326-L348

I also did run the modified fork and have found it behaving correct. I cannot guarantee that anything isn't missed, but I could help with the test cases, should any new be needed.

[cbaker6]

You and Manuel mentioned it here:
parse-community/parse-server#7447 (comment)

In principle, the server already does support the mixed environment and it decides based on createdAt whether to save or create object.

in your comment you state “server” and then say “it”, implying this is done on the server side. Manuel and I were talking about the client SDKs in your link.

with reference to JS SDK , where I understand that the JS SDK also do not check if objectId exists when allowCustomObjectId = true, so what I am proposing is to align this SDK with the JS SDK basically:
https://github.com/parse-community/Parse-SDK-JS/blob/ffc523f9cd0fa2dd3fcc9754d5f15c3c1c78314f/src/ParseObject.js#L326-L348

The SDKs are already aligned. The JS SDK does check for objectID. The lines I think you meant to reference are below: https://github.com/parse-community/Parse-SDK-JS/blob/d196e7f4f861aa76822c59ccb78352fe0bdda80c/src/ParseObject.js#L2402

In my initial response. I referenced parse-community/Parse-SDK-JS#1309 (comment). If you look two comments above, he says parse-community/Parse-SDK-JS#1309 (comment):

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

the comment above is a correct statement which is why I responded to your original comment with: https://community.parseplatform.org/t/both-custom-and-generated-objectid-in-one-project/1812/2?u=cbaker6

This makes sure developers do the correct thing when using custom objectId. Until the conversation is resolved on the JS side, I don’t see a reason to take the SDKs out of alignment as the change can lead to developers submitting issues because they think custom objectId is behaving incorrectly.

[lsmilek1]

I apologise, the SDK is deciding if it is POST or PUT, not the server and indeed I missed the line 2402. You are correct that the SDKs are aligned then.

Perhaps to respond on the comment, I unfortunately found that the mixed behaviour would save in my case:

• back-end fetch and device sync when creating account (save reference between User and Profile)
• improve the client code readability as the client code always need to pass two objectId, instead of one "device user" objectId
• cloud code triggers have to do extra fetch of the Profile or User object to find related partner (above example with setting ACL when there is only profile's objectId known
• extra field at both Profile or User to link them and for some scenarios also indexing

We could argue that my database schema should be different, but as I am not only one who would like to use mixed environment I can imagine multiple use cases out there. So when I was investigating and testing the mixed behaviour, I found it working as intended from the server side (apart from the beforeSave blocking custom objectId bug that I haven't tested yet). One either has to toggle SDK flag (comment) of when I removed the above mentioned checks deciding for PUT/POST.

I agree that it makes sure developers do the correct thing. But I also find that "correct thing" relative, as mentioned in JS discussion it force to use always custom ids and allowCustomObjectId flag tells different. I can confirm that - as a newcomer to parse I understood that flag for a long time that I can use both. So either it should be renamed, or update the SDKs (what I see as small change) added in the documentation that allowCustomObjectId will still generate objectId if it is not defined (together with mentioning that createdAt is needed when allowCustomObjectId if true.

I will continue on the JS side then to clarify the need there. I hope I clarified the need from my side well?

[lsmilek1]

Looking at the commit I understand that the developers will have to set allowCustomObjectId = true and then keep always createdAt either nil or defined to set if the operation creates or updates the object. For each save() that should get objectId assigned from the server there will have to be isIgnoreCustomObjectIdConfig = true.

I wonder if it would not be simpler to keep allowCustomObjectId = true only in server setting and leave it out from SDK, where in SDK would be a parameter in the .save() similar to what are you implementing currently isIgnoreCustomObjectIdConfig = true, for example: .save(createWithCustomId: Bool). That way there would be no need to pay attention to createdAt and SDK's allowCustomObjectId = true. The developers would not be able to override existing data by mistake (or at least not during object creation) and the confusing name allowCustomObjectId would become valid as on the server it would allow both?

I understand this is a bigger change, but it could be implemented together with current solution as an intermediate phase as it would not be in conflict with previous guard statements. What do you think? If you agree I could try to add this functionality.

[cbaker6]

I understand this is a bigger change, but it could be implemented together with current solution as an intermediate phase as it would not be in conflict with previous guard statements. What do you think? If you agree I could try to add this functionality.

It sounds like you are proposing to open a new PR to implement this? If so, feel free. You will also need to add and modify the necessary test cases to assess corner cases.

[lsmilek1]

I will try to continue on the customObjectId branch to keep also your changes. With the test cases I might need some guidance, let's see.

[cbaker6]

You should branch from the main so merge conflicts aren't created