Skip to content

Add identifier syntax #296

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: persisted-documents
Choose a base branch
from
Open

Add identifier syntax #296

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e357f4c
Add identifier syntax
martinbonnin Jul 21, 2024
daa0bec
Update spec/Appendix A -- Persisted Documents.md
martinbonnin Jul 21, 2024
81fa529
more consistent quoting
martinbonnin Jul 21, 2024
bba578d
Update spec/Appendix A -- Persisted Documents.md
martinbonnin Jul 22, 2024
3fbc34c
Remove the 64 chars requirement on sha256 from the parser
martinbonnin Jul 22, 2024
5d48e0e
Use UnreservedCharacter
martinbonnin Jul 22, 2024
014506d
format
martinbonnin Jul 22, 2024
eeab3f3
Feedbacks:
martinbonnin Jul 26, 2024
dda338e
remove parser ambiguity
martinbonnin Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions spec/Appendix A -- Persisted Documents.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,35 @@ implementation specific.
Note: A 32 character hexadecimal _custom document identifier_ is likely to be an
MD5 hash of the GraphQL document, as traditionally used by Relay.

### Document identifier syntax

DocumentIdentifier ::

- PrefixedDocumentIdentifier
- CustomDocumentIdentifier

PrefixedDocumentIdentifier ::

- Sha256HexDocumentIdentifier
- OtherPrefixedDocumentIdentifier

Sha256HexDocumentIdentifier : `sha256:` Sha256Checksum

Sha256Checksum :: 64 LowerCaseHexDigit

LowerCaseHexDigit :: one of

- `0` `1` `2` `3` `4` `5` `6` `7` `8` `9`
- `a` `b` `c` `d` `e` `f`

OtherPrefixedDocumentIdentifier :: `x-` IdentifierCharacter+ `:` IdentifierCharacter+

CustomDocumentIdentifier :: SourceCharacter+

IdentifierCharacter :: SourceCharacter but not `:`

SourceCharacter :: Any Unicode scalar value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incredibly broad; I know that's effectively what we have currently but I feel like we should lock it down a bit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we could allow all Unicode characters, the main spec defines a specific set of characters for use in that document. Something similar could be done here.

SourceCharacter
U+0009
U+000A
U+000D
U+0020–U+FFFF

On a side note, the spec is defining these characters as UTF-16 codes, not Unicode code points.

I’m fine with allowing any Unicode character in this context, or any non control character would be fine too.

Copy link
Contributor Author

@martinbonnin martinbonnin Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had only alphanumericals initially and then backtracked to "any unicode" because "why not". But I think your point about having the identifier in url path is the important one. I'll move to alphanumerical + a bunch of unreserved chars like you did here. Questions:

  • do we even need ~, ., _, -?
  • do we need uppercase letters? This might cause issues on case-insensitive filesystems (hello macOS 👋 )

Edit: updated in 5d48e0e

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly recommend a wide range of characters. URLs can escape any Unicode character so there need be no limit on what characters can be used for document ids. Also, url paths are by definition case sensitive; but they may be treated with no case sensitivity. For instance, if the document id was a guid it would need dashes. If it was base64 it would need lowercase and uppercase and a couple extra characters. If it was readable names, it would need to support Unicode for foreign languages. And a custom implementation may decide to treat the document id with no case sensitivity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the document id was a guid it would need dashes

Excellent point.

If it was base64 it would need lowercase and uppercase and a couple extra characters

Another excellent point. +, / and = would be needed too. Which would make them a bit awkard to use as url path segments... Interesting!

If it was readable names, it would need to support Unicode for foreign languages

I think I disagree on this one. I don't really see anyone using readable names as identifiers? Sounds like a footgun to me that if we can, we should discourage. Just like the relay cursors are encouraged to be opaque, I would encourage the document ids to be similarly opaque.

Tangential: I have a GraphQL-over-HTTP meeting in my calendar on Thursday. Is that still happening? Want to discuss over there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @Shane32; : sits in that grey area where you don't strictly need to escape it, but it's best practice to do so. We should change that to a URL-safe character.

Copy link
Member

@benjie benjie Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the ! suggestion best I think.

I think we can change the spec text to that without making existing : usage non-compliant - it just becomes a custom ID rather than prefixed ID.

Looks like ! gets encoded to %21 via new URLSearchParams({"documentId":"sha256!abcdef"}).toString()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* seems to work with both URLSearchParams and encodeURIComponent, and I don't think it's normally used in encodings so should be safe... I'll need to look up in the relevant specs to see if it's not meant to be used for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec for URLSearchParams:

The application/x-www-form-urlencoded percent-encode set contains all code points, except the ASCII alphanumeric, U+002A (*), U+002D (-), U+002E (.), and U+005F (_).

So ~ is probably encoded also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, yeah. Actually I remember I deliberately chose a character outside of unreserved in RFC3986 for the separator, so I'm okay with : being URL encoded. But * might be a better choice to avoid it.

GitHub renders the URLs okay:


## Persisting a Document

To utilize persisted documents for a request, the client must possess a unique
Expand Down