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

admin-graphql-api-utilities: Improve GID regex and add parseGidObject #2647

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented May 8, 2023

Description

This PR is meant to work in conjunction with #2622 which adds strong typings for GIDs. This PR addresses a comment left in that PR.

One thing I noticed while looking at the code for GIDs was that the regex is not always reliable. It would let many invalid GIDs through:

  • gid://shopify/A/B/C/D/E/F/G/123 - does not validate number of components
  • gid://-_-_-_-/--------/__________ - does not validate identifiers
  • @#$%^&^*()/Foo/123 - does not validate prefix (for parseGid/parseGidWithParams)
  • Etc.

Note

The validity of these GIDs are based on Shopify's documentation, the GID code (which uses in part the URI spec from RFC 2396), and my own understanding/intuition.
The examples above could very well be considered valid for the sake of this package, however, I would strongly suggest for them not to be. They make for more edge cases, added complexity, a harder time improving types, and an increased risk of errors.

To fix these issues, I updated the regex to completely validate GIDs when parsing. This results in the following rules:

  1. Identifiers (namespace and type) should begin with a letter and be followed by zero or more of the following: a-z, A-Z, 0-9, _ (except namespaces), and -. See RFC 3986 for details. This is actually a bit stricter than the URI spec since we only allow alphanumeric characters and hyphens (no +, ., or other typically allowed character).
  2. The prefix (gid://) must exist.
  3. There may only be three components: namespace, type, and ID (with optional query string).
  4. Partial IDs are no longer accepted (i.e. you can't parse "12345" as a GID).

Changelog

Added

  • Added parseGidObject to parse a GID string into an object with the relevant components

Changed

  • Changed what is considered a "valid" GID based on what Shopify, rails/globalid, and the URI spec suggest
  • parseGid and parseGidWithParams no longer accept partial GIDs

Open Questions

  1. Should we remove parseGidType and parseGid, replacing them with parseGidObject (possibly renaming it to parseGid)?
  2. Should we parse the query string in parseGidObject? I chose not to so we save on wasted operations for those who just want the ID or type. However, we definitely could do that.

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

Successfully merging this pull request may close these issues.

None yet

1 participant