WIP: Improve parser for WAC Allow header #1326

ThisIsMissEm · 2021-11-08T20:30:21Z

This new parser is more thoroughly tested, and also has a small efficiency gain: we only parse the header once, and then make assertions on it, rather than parsing once for each scope group.

There's not yet a ticket for this work, as it was just an improvement I saw that we could make to improve code quality (I wasn't working on anything else more important today whilst I was waiting for feedback on scheduled work)

Related work: https://github.com/solid/wac-allow

Checklist

All acceptance criteria are met.
Relevant documentation, if any, has been written/updated. not sure if this needs updates, as it's an internal API
The changelog has been updated, if applicable. not sure if this needs updates, as it's an internal API
New functions/types have been exported in index.ts, if applicable. n/a
New modules (i.e. new .ts files) are listed in the exports field in package.json, if applicable. n/a
New modules (i.e. new .ts files) are listed in the typedocOptions.entryPoints field in tsconfig.json, if applicable. n/a
Commits in this PR are minimal and have descriptive commit messages.

This new parser is more thoroughly tested, and also has a small efficiency gain: we only parse the header once, and then make assertions on it, rather than parsing once for each scope group.

codesandbox-ci · 2021-11-08T20:31:27Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit dafe51d:

Sandbox	Source
solid-client-sandbox	Configuration

ThisIsMissEm · 2021-11-08T20:34:00Z

src/resource/wacAllow.internal.ts

+ */
+
+// TODO: Should we move this type internally, as the Access in this file is not strictly the same as ACL Access
+import { Access } from "../acl/acl";


@NSeydoux @VirginiaBalseiro what're your thoughts on this one? I think parsing WAC Allow shouldn't be re-using data structures from ACL, as they are different things (the types just happen to currently be compatible)

Well it kind of makes sense that the Access definition defined in ACL is used for the WAC-Allow header, given that the WAC-Allow header was initially described in the ACL spec. We have another Access type definition from the access/universal submodule for universal (i.e. for both ACL-controlled and ACP-controlled Pods), which is somewhat different (because ACP has a different notion of Control than ACL), but could maybe used here, since the WAC-Allow header is now present in both ACL and ACP specs.

Would it make sense to rename Access in ./acl/acl to AclAccess (we would need to export it still as Access but mark that as a deprecated name (that'd give interface/type naming consistency within Acl

Then have wacAllow use a internal_Access type that's compatible? There's also a WacAccess in https://github.com/inrupt/solid-client-js/blob/c8da4e5b0a7baa20ff04558b7e06d1a42e8df3ae/src/access/wac.ts — I'm not sure what this is exactly, and it's undocumented in the currently released SDK version, as far as I can tell.

Perhaps we move the WacAccess up to a ./wac-allow directory, and move WacAccess and the parser / transform there?

The Access interface (there is no reason for it to be a type considering its declaration) should in my opinion:

Be moved to a type folder under src: /src/type/

Have all the modes

Be used everywhere such Objects are inputs/outputs (we need to remove all the aliasing and other similar definitions)

It would be particularly convenient to have one place where all the standard interfaces are defined.
We really need to remove all the aliasing going on, it is exactly the opposite of what interfaces are for. You don't want 20 names for the same thing, you want 1 name for a specific shape and as few of them as required.

As a sidenote, the ACP specification will adopt the standard Solid access modes shortly, that is, use the acl:Control mode just like WAC. It is a design flaw that it currently invents another mechanism. The operations entailed by Access Modes are defined at the Solid protocol level.

ThisIsMissEm

Highlighted a few areas in the diff that might be of interest

ThisIsMissEm · 2021-11-08T20:35:30Z

src/resource/wacAllow.internal.ts

+  return entries;
+}
+
+function internal_getWacPermissions(


Not sure this is named correctly / strictly belongs here; perhaps internal_getPermissions would be better, as it's just checking a parsed WAC Allow for given values

Sounds good to me, we can always move it if it becomes relevant elsewhere.

ThisIsMissEm · 2021-11-08T20:36:43Z

src/resource/wacAllow.internal.ts

+
+  return {
+    read: permissions.includes("read"),
+    append: permissions.includes("append") || permissions.includes("write"),


This is the same logic as, but simpler than: https://github.com/inrupt/solid-client-js/blob/main/src/resource/resource.internal.ts#L87-L100

I think the way https://github.com/inrupt/solid-client-js/blob/main/src/resource/resource.internal.ts#L87-L100 is written was motivated by type definitions, if I recall correctly the TS compiler was complaining if the appendand write values were not constants, but this looks definitely simpler.

ThisIsMissEm · 2021-11-08T22:01:38Z

I'm not sure why CD / cd-end-to-end-tests (14.x) (push) Failing after 11m — cd-end-to-end-tests (14.x) is failing — the output doesn't make a lot of sense to me.

NSeydoux · 2021-11-09T08:41:13Z

src/resource/wacAllow.internal.ts

+ */
+
+// TODO: Should we move this type internally, as the Access in this file is not strictly the same as ACL Access
+import { Access } from "../acl/acl";


Well it kind of makes sense that the Access definition defined in ACL is used for the WAC-Allow header, given that the WAC-Allow header was initially described in the ACL spec. We have another Access type definition from the access/universal submodule for universal (i.e. for both ACL-controlled and ACP-controlled Pods), which is somewhat different (because ACP has a different notion of Control than ACL), but could maybe used here, since the WAC-Allow header is now present in both ACL and ACP specs.

NSeydoux · 2021-11-09T08:52:25Z

src/resource/wacAllow.internal.ts

+  return entries;
+}
+
+function internal_getWacPermissions(


Sounds good to me, we can always move it if it becomes relevant elsewhere.

NSeydoux · 2021-11-09T08:56:35Z

src/resource/wacAllow.internal.ts

+
+  return {
+    read: permissions.includes("read"),
+    append: permissions.includes("append") || permissions.includes("write"),


I think the way https://github.com/inrupt/solid-client-js/blob/main/src/resource/resource.internal.ts#L87-L100 is written was motivated by type definitions, if I recall correctly the TS compiler was complaining if the appendand write values were not constants, but this looks definitely simpler.

Improve parser for WAC Allow header

dafe51d

This new parser is more thoroughly tested, and also has a small efficiency gain: we only parse the header once, and then make assertions on it, rather than parsing once for each scope group.

ThisIsMissEm commented Nov 8, 2021

View reviewed changes

github-actions bot deployed to review November 8, 2021 20:38 View deployment

NSeydoux self-requested a review November 8, 2021 21:30

NSeydoux approved these changes Nov 9, 2021

View reviewed changes

ThisIsMissEm marked this pull request as draft February 1, 2022 19:08

ThisIsMissEm changed the title ~~[WIP] Improve parser for WAC Allow header~~ WIP: Improve parser for WAC Allow header Feb 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Improve parser for WAC Allow header #1326

WIP: Improve parser for WAC Allow header #1326

ThisIsMissEm commented Nov 8, 2021 •

edited

codesandbox-ci bot commented Nov 8, 2021

ThisIsMissEm Nov 8, 2021

NSeydoux Nov 9, 2021

ThisIsMissEm Nov 10, 2021

matthieubosquet Nov 17, 2021

ThisIsMissEm left a comment

ThisIsMissEm Nov 8, 2021

NSeydoux Nov 9, 2021

ThisIsMissEm Nov 8, 2021

NSeydoux Nov 9, 2021

ThisIsMissEm commented Nov 8, 2021

NSeydoux Nov 9, 2021

NSeydoux Nov 9, 2021

NSeydoux Nov 9, 2021

WIP: Improve parser for WAC Allow header #1326

Are you sure you want to change the base?

WIP: Improve parser for WAC Allow header #1326

Conversation

ThisIsMissEm commented Nov 8, 2021 • edited

Checklist

codesandbox-ci bot commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMissEm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMissEm commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMissEm commented Nov 8, 2021 •

edited