Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional URN prefixes are not being parsed correctly. #8

Open
jaylattice opened this issue Feb 29, 2020 · 5 comments
Open

Optional URN prefixes are not being parsed correctly. #8

jaylattice opened this issue Feb 29, 2020 · 5 comments
Labels

Comments

@jaylattice
Copy link

Hi again, @mike-marcacci! I was unsure how to solve this problem, and the out-of-repo fix is quite trivial, so I did not open a PR. That said, I would still like to inform you of this issue.

Based on the ABNF rules for attrPath (leaving out the other tokens for brevity, you get the idea):

attrPath  = [URI ":"] ATTRNAME *1subAttr

An attrPath may contain an optional URI, followed by a colon. However, trying to parse an example does not work:

import { parseAttributePath } from "scim-query-filter-parser";
const filterPredicate = parseAttributePath("urn:ietf:params:scim:schemas:core:2.0:User:userName")
// Error: Failed to parse!

I suspect this is an error in either your transformation of the ABNF rules, or in apg-lib. The problem can be easily solved out-of-repo by slicing off the URI before passing the purported attrPath to parseAttributePath. It's a very simple fix, but it would be awesome if you looked into why this is happening.

@jaylattice
Copy link
Author

On a side note, I was wondering if you were able to find a tool online that, given two ABNF files, checks for their mathematical equality. I was only able to find Bill's ABNF validator, but this just validates and canonicalizes the ABNF; it does not check for equality given two ABNF files. If we had this tool, it would be easy to see if the ABNF transformations you did are compliant.

@mike-marcacci
Copy link
Member

Thanks for the note here: I'll definitely investigate this.

I'm not aware of a tool that does algebraic comparisons of ABNF files, although it should be possible. I would be very interested if you found one! I suspect my transformation to remove left recursion (which is unsupported by just about all parsers) would trip up naive implementations.

@jaylattice
Copy link
Author

Cool, thanks for the speedy response! For now, this isn't a blocker for us as our regexp-based fix is sufficient. I'll keep you posted if I find anything on the algebraic equivalence, though. I would be surprised if such a program did not exist!

@jaylattice
Copy link
Author

jaylattice commented Mar 2, 2020

Hmm. I am actually led to believe this is an apg-lib bug! For example, I recompiled the library with the following change to the grammar:

attributePath = [URI "<"] attributePathSegment *1("." attributePathSegment)

(i.e. change the optional delimiter from ":" to "<")

And now it can correctly parse stuff like:

import { parseAttributePath } from "scim-query-filter-parser";
// Note the delimiter was changed from ":" to "<"
const filterPredicate = parseAttributePath("urn:ietf:params:scim:schemas:core:2.0:User<userName")
// ["userName"]

It doesn't include the URI in the output array because I didn't update the library-level code to consider the current in-context URI in Yard, but more importantly, the parsing itself does not fail.

I'm taking a wild stab in the dark here, but it appears the ":" delimiter is confusing apg-lib because a valid URI can also end in a ":". Maybe apg-lib is not equipped to deal with this ambiguity.

@jaylattice
Copy link
Author

Another example illustrating this issue:

attributePath = attributePathSegment "-" attributePathSegment
attributePathSegment  = ALPHA *("-" / "_" / DIGIT / ALPHA)

I guess this is ambiguous because "some-path-segment-other-path-segment" has more than one correct answer. It could parse to [ "some-path-segment", "other-path-segment"] or the equally valid ["some-path", "segment-other-path-segment"]. I hope this is helpful; I'm not quite sure if ABNF has a way to specify "last occurring element".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants