Add SCIM V2 PATCH compilePath support; fix bug in ABNF file.

[jaylattice]

Hello. First, thanks for writing this amazing library! I learned a lot from reading your code. I work at Lattice and we're implementing a SCIM V2 server and require basic path parsing support. Properly parsing the path is important to us because it makes conditional behavior easier to do (e.g. throw a SCIM 501 if the path uses a filter and we don't support filters, throw an error if we don't support certain fields, etc.).

This PR:

1. Updates the grammar.abnf file and fixes a bug with it.
   • Bug was that we were allowing arbitrarily nested sub-attribute matching.
2. Adds a compiler for the PATCH path (e.g. 'emails[type eq "work"].value')

We need this compilePath function so that we can extract filters and nested attribute chains from paths so we can more confidently support SCIM V2 PATCH and more easily decide when to throw SCIM 501 errors.

Future ideas:

I noticed a way to generalize this library. Instead of hard-coding the compilers into the existing Yard tracker, we can make compilers pluggable so that compilePath, compileFilter, and compileSorter then just become plugins to the parser. This would be useful for anyone who needs to extract other types of data from the parsed output. For example, in our use-case, the current filter predicate ((any) => boolean) requires us to fetch all the users a priori in order to filter them.

One could instead imagine a plugin that compiles a filter to an AST (btw, I noticed you did this in your V1 of this library 😄) so that it can be processed by some SQL generator or custom filter conversion logic or something. I would be happy to submit PRs for this.

[jaylattice]

This was the bug; please see RFC 7644 3.4.2.2. In short, we need an upper bound of 1 on the number of allowed attributePathSegment matches.

[mike-marcacci]

Indeed, this was actually a deliberate deviation from the spec for an internal use-case, and I should have documented the difference. However, the use-case has since disappeared so it's probably appropriate to revert to spec-compliant behavior (even if I personally dislike the arbitrary depth limit).

Because this deviation was never documented I'll consider this as a "bug fix" and not a major breaking change, as we shouldn't have been depending on this behavior.

[jaylattice]

I used parenthesis here because apparently apg-lib's precedence rules are contradictory to RFC 5234 3.10.

[mike-marcacci]

Fascinating and great catch on the precedence bug.

[mike-marcacci]

First, thank you for the incredibly high-quality PR! I do have some concerns about the library returning data for some cases and executable code for others (see my comments on Yard.ts)... but I'm obviously willing to deal with this if it turns out to be the most useful strategy.

On another note, I certainly support the idea of making the compiler more "pluggable" such that it could be used for things like SQL generation or construction of intermediate representations. Perhaps this is a solution to my hesitation, as both "data returning" and "function returning" compilers could be implemented...

[mike-marcacci]

Fascinating and great catch on the precedence bug.

[mike-marcacci]

Indeed, this was actually a deliberate deviation from the spec for an internal use-case, and I should have documented the difference. However, the use-case has since disappeared so it's probably appropriate to revert to spec-compliant behavior (even if I personally dislike the arbitrary depth limit).

Because this deviation was never documented I'll consider this as a "bug fix" and not a major breaking change, as we shouldn't have been depending on this behavior.

[mike-marcacci]

I'm going to have to think about this one. Version 2 of this library is built around the paradigm of returning code that "executes the query" rather than returning "information about the query".

The reason for this is to maximize the amount of spec compliance this library can provide (traversing the object correctly and safely, for example, is not exactly trivial).

Instinctually, I would rather return set and get functions which could be used by your code to modify the object (or perhaps return a modified copy of the object)... but I also don't want to force the pattern here when doing so makes the library less usable.

Would this kind of strategy work for your use-case?

[mike-marcacci]

How would you feel about an API that compiled a PATCH path into the following:

compilePatch(input: string): function<T=unknown, M=unknown>(
	data: T,
	modifier: (before: M, path: string[]) => M
): T

This would allow your modifier to make any path-specific decisions without having to actually traverse the object. The library could also enforce a copy-on-write policy such that the original data object is never modified.

[jaylattice]

I'm going to have to think about this one. Version 2 of this library is built around the paradigm of returning code that "executes the query" rather than returning "information about the query".

V3 "Pluggable compilers" could be built around the paradigm of supporting arbitrary paradigms! We could do a minor refactor, then ship this library with some out-of-the-box compilers including:

// "safe, avoid footgun, at the expense of performance" paradigm.
compileFilterToExpression() // what V2 does
// less safe, but more versatile
compileFilterToAST() // what V1 did
compileAttributePathToFieldArray() // what V2's parseAttributePath does

This would not only support all existing functionality in V1 and V2, but also allow devs like me to implement custom compilers without submitting PRs!

[jaylattice]

As for your example, I'm not sure what you mean with modifier. Do you mind elaborating with a simple example of how that would be used by the caller? I would imagine, following the same "executable" pattern of compilerFilter, compilePatch would be used like:

// simplified for brevity
const myUserResource = getSCIMUserResource(...);
const patchOps  = req.body.Operations;
for (const op of patchOps){
  try {
    const applyPatchOp = compilePatch(op)
    applyPatchOp(myUserResource, true) // true to mutate
  } catch (err) {
    res.status(400).json(SCIM_INVALID_PATCH_OP_ERR)
    return;
  }
};
// Cool! Our user resource is now patched.
// Update DB, send result to client
updateDB(myUserResource);
res.status(200).json(myUserResource);

I'm not sure how much extra work that would be on this PR, but my intuition says it is not trivial because of all of the little rules around what constitutes valid SCIM patch ops, and how they get applied. My current solution is:

1. Parse the patch ops using compilePatchPath in this PR.
2. Apply SCIM-centric validations.
3. Transform it into something that can be consumed by a JSON patch library.
4. Apply the JSON patch to the document.

[jaylattice]

I certainly support the idea of making the compiler more "pluggable" such that it could be used for things like SQL generation or construction of intermediate representations. Perhaps this is a solution to my hesitation, as both "data returning" and "function returning" compilers could be implemented...

Glad to see we're on the same page! I left a related, opinionated comment in this issue because I see that a pluggable solution could also help prevent issues like this and encourage people to build solutions on top of this library.

[Sherri010]

Hi @mike-marcacci, just wanted to follow up on these changes. Are you planning on merging this PR? We love for these updates to be included in the package so we don't have to maintain our own fork.

[sonarcloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud