Update old records by ID will override default ACL

[chpapa]

• Skygear Server Date/Version:
• Application Platform: JS, potentially iOS and Android too?
• Is this a regression?
• Attached logs, screenshots

Expected Results

Record updated only with the new values

Actual Results

Record's ACL will be overrides by SDK default

Steps to reproduce

Reported by users, Our guide show a shortcut to update a old record

skygear.publicDB.save(new Note({
_id: 'note/<your-note-_id>',
content: 'Hello New World'
}));

[b123400]

This issue is a bit tricky. Currently we only have 1 API for both creating and updating, in the above example, the client has no way to tell which one is it. Therefore your expected result, "Record updated only with the new values" is impossible as there is no information on "is the ACL value new".

The main problem is the fact that the default ACL can be specified in Record.defaultACL.

Consider this case:

Record.defaultACL=/* ACL1... */;
save(new Note("Note/1"))
Record.defaultACL=/* ACL2... */
save(new Note("Note/1"))

Which result do you expect?

• If defaultACL means the default value on server side, ACL1 is correct.
• If defaultACL means the default value on client side, ACL2 is correct.
• The current interface seems to be ambiguous and does not specify what do we want.

Choices:

We have the following choice:

1. If an _id is provided, and the record is created locally, send an extra query to fetch the record from the server before save, so we will know what should be overridden
2. Make it clear what does defaultACL means
3. Disallow changing Record.defaultACL

---

I think 1 is better.

[chpapa]

I think Choice 1 above could solve the problem, but it also defeat the purpose of the shortcut -- to avoid sending the extra query. I think it is acceptable but not ideal?

Can we also consider the following options?

4. Kind of similar to 2, we clarify defaultACL only meant if a record is new, the default ACL will be applied; So the defaultACL is pass to server side, but the server side will always leave it alone if the Record ID already exists; If the Record ID does not exist, it will apply the default ACL from the client side. I guess it will involve adding one more parameter in the ACL record to state if it is a "defaultACL" or a "customACL"? Would need to think how to add this information in without breaking anything.

But I guess the new definition of defaultACL would be easier to understand and less surprising for developers?

[chpapa]

@b123400 It just suddenly cross my mind, we shall also make sure the new change have the following expected behaviour:

1. Query RecordA from Server
2. Change RecordA._id = new UUID
3. Save ReacordA to Server

There are some codes doing it now, and the behaviour is equal to duplicate RecordA

I think people who wrote the above code would expect the ACL of old RecordA and new RecordA are identical.

One idea is, as mentioned above, we have a special field in defaultACL from client side to tell server side that "it is the defaultACL if you're going to save a new Record", and the client side should well aware that a defaultACL is NOT permanent until it is saved.

Other idea are welcomed, just want to point out another case.

[b123400]

That is possible. And I think we don't need any special handle for "changing id to copy duplicate record" to works, it will just work

[chpapa]

@b123400 Cool. Do you think we can add the above two cases (duplicate records by changing ID will keep existing ACL, and update existing records with the new Record with existing ID shortcut will preserve ACL unless explicitly setted) as Test cases for regression?

[b123400]

Yes sure, I have a working version locally, will send PR when test cases are ready~

…

Sent from my iPhone

On 17 Mar 2017, at 16:01, Ben Cheng ***@***.***> wrote: @b123400 Cool. Do you think we can add the above two cases (duplicate records by changing ID will keep existing ACL, and update existing records with the new Record with existing ID shortcut will preserve ACL unless explicitly setted) as Test cases for regression? ― You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[rickmak]

The main problem is the fact that the default ACL can be specified in Record.defaultACL.

The design is assuming the client side have the knowledge on almost everything. Including how should the ACL behave, is the record new or old. So my expected usage is user override the access attribute on both create/update.

If an _id is provided, and the record is created locally, send an extra query to fetch the record from the server before save, so we will know what should be overridden

If we stick with the assumption. The developer should already know the record is new or old. It should already queried the record at some point of time before he actually want to create/update the record attributes.

Problem with #322 and SkygearIO/skygear-SDK-JS#175

• save is idempotent now. Will break it.
• It create one extra attributes in API and more complex save logic.

Above just background, following is my suggested solution:

This issue is telling the assumption don't hold. In real world, developer don't have the whole record when updating. i.e. developer actually don't cache/persist the knowledge we assume he have. (Developer just get the uuid from some source or hard-code the id?)

So let remove the assumption.

Without the assumption. The defaultACL should be server-side knowledge.

So the defaultACL will become something like setRecordCreateAccess call. It actually setting the server state and setting. Not an SDK configuration.

The logic will be:

1. If SDK pass in ACL value explicitly, it will always be the final value. For both create/update.
2. If the SDK don't pass in ACL value, the record will be have default ACL if the existing value is null. For both create/update. In the case of create, the existing value will always be null.

[chpapa]

How shall we handle the transition of old SDK without breaking too much? Seems existing SDK allow a defaultACL set at client side right?

[rickmak]

This issue is not implemented at Android SDK. refs: SkygearIO/skygear-SDK-Android#70

[chpapa]

@rickmak iOS and JS are implemented?

And if Android SDK are not implemented, does that meant the latest version of skygear-server can't be used with latest version of Android SDK? If yes we have to fix it...

[b123400]

Android SDK should work exactly same as the last version, the API is backward compatible

[rickmak]

Prior 0.23, the user of Android SDK need to explicit specify ACL per record or it will become NULL(publicly accessible). After 0.23, missing ACL will populate the server default value if any.