file.acl.get malformed request if entity is not provided

[theGOTOguy]

Environment details

• OS: Alpine
• Node.js version: 18.12.0
• npm version: 8.19.2
• @google-cloud/storage version: 6.9.0

Steps to reproduce

1. Create a file in a bucket.
2. Run

let fileAcls = [];
await file.acl.get().then((data) => {fileAcls = data[0]});
console.log(fileAcls);

3. Note that, while entity (a required option!) was not provided, we listed all ACLs for the file.
4. Change your bucket to requester pays.
5. Run

let fileAcls = [];
await file.acl.get({userProject: "YOUR_PROJECT_HERE"}).then((data) => {fileAcls = data[0]});

6. Note malformed request error.

The reason for this is here. If entity is not provided but any other option is, then our path is /undefined. On the other hand, if no options at all are provided, entity effectively defaults to ''. The issue can straightforwardly be worked around by providing the option {entity: ''}.

I propose one of two solutions:

1. If entity is not provided, default entity to empty string. This effectively makes entity not a required field.
2. Enforce entity as a required field. Fail file.acl.get(...) if entity is not provided. Document that users wishing to retrieve all ACLs should pass {entity: ''}.

[ddelgrosso1]

Thank you for opening an issue @theGOTOguy. I am assuming you are attempting this from a JavaScript application and not TypeScript? When I attempt this in a TypeScript I'm not even able to compile due to the following:

    Argument of type '{ userProject: string; }' is not assignable to parameter of type 'GetAclOptions'.
      Property 'entity' is missing in type '{ userProject: string; }' but required in type 'GetAclOptions'.

Additionally, in looking at the JSON API docs this is a documented required field. We generally shy away from adding additional error checking on top of what the API returns.

However, it does appear that the docs for ACL are missing from the client docs and I will look into the reason behind that.

[theGOTOguy]

Yes, I am using JavaScript and not Typescript.

I agree with you that the client is not the right place to do error checking for the API. In vanilla JavaScript, the client defaults a different value for entity depending on whether other options are included or not which is certainly less desirable than either setting it to a consistent default or failing the request entirely for missing a default field.

[ddelgrosso1]

Correcting what I said above, I did find the client docs, however they appear incorrectly labeling entity as optional. I will fix this.

[ddelgrosso1]

Do you have the stack trace of the error? I assumed this was something bubbling up from the API side, if not I can look at correcting it in the client.

[theGOTOguy]

I agree that to make entity required would be an acceptable fix, but I wanted to add a few concerns you might want to consider before considering this closed.

1. Other existing Google documentation shows (correctly) that file.acl.get() will work even if entity is not provided in Node.JS, so you will want to update that as well.
2. Clearly, this change is documentation-only. It will not be enforced in code and will not break existing code. This leaves open the possibility that other users will, like me, find themselves confused when they pass non-entity options and the default value for entity changes. Without going through the trouble of setting up a bucket, creating a file, getting a key to connect and running the code through a node console: it's easy to see tracing through the code how this happens.

ben@anteater:~$ node
Welcome to Node.js v17.2.0.
Type ".help" for more information.
> '/' + encodeURIComponent(undefined)
'/undefined'

What's happening is that when entity is not provided, if any other option is provided, then we hit this line and the default value of entity provided by your client becomes 'undefined' instead of '', as it is if no other option is provided. I would suggest that the lowest-friction path forward would be to simply check for the entity option in the client and keep the current default, '' if entity is undefined. That way you have no breaking changes and you keep the existing, sensible default.
3. It may be worth documenting somewhere that entity: '' will retrieve all ACLs.

[ddelgrosso1]

To clarify a bit further file.acl.get() with no arguments is a list operation while file.acl.get({arguments}) is a get operation.

Under the current code if a user does not supply the entity argument or supplies it incorrectly the server returns a 400 Bad Request. By defaulting entity = '' it is essentially converting the get back into a list which I'm not convinced is appropriate as the user may not be intending this.

The documentation does say that supplying no arguments retrieves all acls but stops short of saying that it is essentially 2 different API calls without parameters vs with parameters. This get / list pattern is pretty consistent throughout the library so I'm not sure changing it in one place makes sense. That said, if helpful we could look at more explicitly calling this out in the documentation.