setReadWriteAccessForRole on new record result in TypeError: this.access is null

[rickmak]

• Skygear SDK Date/Version: 0.23.0
• JS runtime: browser
• Skygear Server Date/Version:
• Is this a regression?
• Attached logs, screenshots

Expected Results

Set the ACL correctly.

Actual Results

TypeError: this.access is null

Steps to reproduce

var NewsModel = $skygear.Record.extend('news');
var news = new NewsModel();
news.setReadWriteAccessForRole(CmsAdminRole);

[cheungpat]

Need a test case for this one too

[b123400]

Because now default ACL is in server side, we need to do something like

setReadWriteAccessForRole = (role)=>
   fetchDefaultACL(this.type)
   .then(acl => acl.setReadWriteAccessForRole(role))

Implemented an endpoint for fetching default ACL, JS updates coming

[cheungpat]

@b123400:

setReadWriteAccessForRole = (role)=>
   fetchDefaultACL(this.type)
   .then(acl => acl.setReadWriteAccessForRole(role))

Do you suggest this should be the behavior for all other SDKs?

[chpapa]

The above code should be run only when access is null? When access is not null it could meant the record was fetched and shouldn't override with default ACL?

Without much modern JS experience so just a guess. Or another way of speaking is I think the access properties should replaced with a getter and fetch default if null?

Also what if the sdk was used at offline environment? The SDK should be usable in offline.

[rickmak]

Let look back to the issue trigger the change: SkygearIO/skygear-server#309

Main point: We removed the assumption of developers having whole record knowledge.
However, we still keep our SDK call is overriding in the previous discussion.

I think that why we still have setDefaultACL?

So before adding the fetchDefaultACL, we need to design how the API to be used. And consider the following:

• What is the relation between setRecordDefaultAccess and setDefaultACL?
  • If we still keep SDK call overriding, we can still keep setDefaultACL and let existing user keep their usage?
  • We may consider merging this two call, suggesting the user call setRecordDefaultAccess every time they bootstrap their app.
• It is not good to make a network call fetchDefaultACL every time. We will need caching. How will the cache be done? How will we explain it to user?
  • Are we going to introduce a bootstrap cycle at client SDK?

my suggested solution

• Merge setRecordDefaultAccess and setDefaultACL
• Suggest the user to either call setRecordDefaultAccess or fetchDefaultACL before any ACL operation.
• Missing call of setRecordDefaultAccess or fetchDefaultACL will infer existing default ACL, i.e. public read.

doc

I also discover the new release is not yet reflected to https://docs.skygear.io/guides/cloud-db/acl/js/

[rickmak]

run only when access is null?

sync call and async call is not capitable in JS. So language wise, it is not possible. Or you will need to add many flag to make it happen and the code base wiil become not maintable.

The SDK should be usable in offline.

What do you mean?? Current SDK cannot save record when the device is offline...

[chpapa]

What do you mean?? Current SDK cannot save record when the device is offline...

Sorry I meant to say it should be able to handle no / bad network situation at best effort, thus calling fetch default ACL Everytime is no good. Your suggestion of caching in last message would be acceptable to me.

Suggest the user to either call setRecordDefaultAccess or fetchDefaultACL before any ACL operation.
Missing call of setRecordDefaultAccess or fetchDefaultACL will infer existing default ACL, i.e. public read.

I think these two are not good enough. Users don't read docs in details as we know. Can we just cache the defaultACL and update it when called manually / times when it is used and in good network condition?

[cheungpat]

My suggestion is that:

• Setting record attributes (including access control) should not cause network request to happen whatsoever.
• Network request is not required before setting record attributes.

Therefore this is my suggestion for implementation:

• New record should have null ACL upon creation at the client side.
• Server will set null ACL to the default ACL upon saving.
• Client should maintain ACL settings if it is not explicitly changed by the developer.
• Changing record ACL in the client side:
  • if originally set to null, it is treated public read + the change
  • if originally set to non-null, it is treated as the previous settings + the change

[cheungpat]

I don’t see why we should treat the access control attribute any differently than any other record attributes when saving.

If the client side does not specify a value, the server decides the value. If the client side specified a value, the client side will override whatever value existing on the server.

On the other hand, I am okay with fetching default access control from the server. I am just against automagically setting the ACL on new record when client creating a new record.

[chpapa]

@cheungpat in your suggestion, what would skygear.setDefaultACL(acl); meant? If new record would not have the defaultACL upon creation? Just worried it would break some existing apps.

[cheungpat]

I am okay with @rickmak’s suggestion to merge setRecordDefaultAccess and setDefaultACL. In other words, the developer is expected to call setRecordDefaultAccess and we should remove setDefaultACL.

The setRecordDefaultAccess only concerns with setting the server-side record default access settings. The client side does not need to cache this setting. (But we will provide the user with the API to query the default access settings anyway).

If new record would not have the defaultACL upon creation?

Let me clarify that new record will not have default ACL (meaning access control setting is null) when the record is instantiated in the client side. For illustrations, this is my proposed events when creating records:

• Client instantiates a new Record, the access control setting is null
• Client call record:save on the new Record (send to server)
• Server sees access control setting is null, it find the default access setting for this record type
• Server sets the default access setting to the record
• Server save the record to the database with default access setting.
• Server returns to client the new Record with the default access control settings.

[cheungpat]

Just worried it would break some existing apps.

Don’t worry. If the developer calls setRecordDefaultAccess instead of setDefaultACL, the developer gets what they want after they have saved the record to the server. The only thing that is breaking is if the developer want to get the ACL settings before the record is saved for the first time.

Since the ACL is only useful at the server side, having ACL setting as null before the record is saved for the first time will not pose much problems.

[b123400]

It was a mistake I didn't remove setDefaultACL, the assigned value is not used anyway.

Re rick's suggestion on asking user to call fetchDefaultACL(), I think it is very difficult to ask user to call a function, if we are doing it we have to make it automatic

[cheungpat]

I will be reviewing the ACL settings (sorry), please take a look at @b123400 and see if you want to make a proposal there.

SkygearIO/skygear-server#359

[cheungpat]

This will be superseded by #208 but we should keep this open to verify that this issue is fixed.