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

Options to remove base64 ID encoding at compile time #229

Merged
merged 1 commit into from
Apr 23, 2022

Conversation

AndrewLipscomb
Copy link
Contributor

@AndrewLipscomb AndrewLipscomb commented Mar 24, 2022

This one is a bit of a bigger one - we've been looking at other GQL implementations in other languages, and noticed that they don't encode Base64 for the ID type.

While I agree that Base64 IDs are actually better given their implicit binary nature - we need to get consistency between the other implementations we are building up.

Given the scope of changes and breakage for existing client usage - I've added an option as a compile time flag for the behaviour change.

Obviously - the library user would need to ensure that they are not encoding anything that is illegal here - but given its an opt-in - I would expect its more a "user-beware" sort of option

@ghost
Copy link

ghost commented Mar 24, 2022

CLA assistant check
All CLA requirements met.

@wravery
Copy link
Contributor

wravery commented Mar 30, 2022

I support the idea of making it optional, but I want to investigate a little to see if there's a way to make a runtime determination, e.g. holding a std::variant<std::vector<uint8_t>, std::string> instead of just a std::vector<uint8_t>.

Much like the JSON parser sets a flag to accept strings as possible enum values, ID scalars should do the same, and clientgen should either skip decoding the Base64 string, or handle the exception for cases where it receives a parsed JSON response::Value rather than an in-memory response::Value with the original std::variant.

@wravery
Copy link
Contributor

wravery commented Mar 30, 2022

BTW, the original decision to always encode ID scalars as Base64 came from the "Learn GraphQL" tutorial, and not the spec. The spec actually indicates that it should be OK to encode any type, e.g. an integer as the ID, as long as it's serializable to and from a string. So in retrospect, ID should have maybe been just an alias for String.

@AndrewLipscomb
Copy link
Contributor Author

but I want to investigate a little to see if there's a way to make a runtime determination

I guess there would be, and regex validating an ID string to see whether its a base64 match isn't hard - the big things I could see are

  • False positives on strings that fit the pattern of a base64 ID, but aren't actually base64
  • IDs that are literally meant to be base64 IDs - not necessarily parsed out as such but passed-as-is to the backend (Can't say that I know of any products that work that way - but couldn't rule it out)

I would have thought the safest option would be to let the user decide

@wravery
Copy link
Contributor

wravery commented Apr 19, 2022

I would have thought the safest option would be to let the user decide

Yes, but I think it should be possible to make that determination per-resolver/field implementation, rather than globally at compile time.

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.

2 participants