Skip to content

Use urlsafe_encode64 for object Id's#2106

Closed
jgodson wants to merge 2 commits intormosolgo:masterfrom
jgodson:master
Closed

Use urlsafe_encode64 for object Id's#2106
jgodson wants to merge 2 commits intormosolgo:masterfrom
jgodson:master

Conversation

@jgodson
Copy link
Copy Markdown

@jgodson jgodson commented Feb 13, 2019

Uses Base64Bp.urlsafe_encode64 encoding for Node id's so that they are safe for use within url's.

We have been using a generated id returned from graphql for unique identification in a route (eg: /app/SW1wb3J0LTY3) and in order to safely do this, we needed to make this change. I thought I'd try to contribute it back as it seems like a nice default to use the url safe version if anyone else has done/wants to do the same.

@rmosolgo
Copy link
Copy Markdown
Owner

Hi! Thanks for upstreaming this. I really wish I had had the foresight to put this default from the very start, but alas, I didn't 🤦‍♂️ .

I totally agree that URLsafe base64 is better, but here's my hangup: if we change the default in GraphQL-Ruby, it might break people's IDs who are using this encoder. I'd hate for someone to bump the gem but accidentally break IDs across their system.

So, how can we improve the default without giving that unpleasant surprise?

  • Wait until graphql-ruby 2.0. I think it would be fair to make this change there, including a compatibility snippet in the changelog for people who want to keep the previous behavior
  • Instead of modifying the existing encoder, we could add a new, URL-safe encoder that people could choose. We could update the Rails generators to recommend the new encoder for new apps and mark the existing one as deprecated. (And even make the new one default in 2.0, as described above.)

What do you think, would one of those approaches do the job?

@jgodson
Copy link
Copy Markdown
Author

jgodson commented Feb 13, 2019

Yes, absolutely, wouldn't want this to break anything. Either of your suggestions are great!

My thoughts on the second is that I wonder if it's worth the time to do vs just waiting until 2.0 and making it the default? I don't know if there's a good reason, beyond backwards compatibility, to use the existing encoder?

If there is, then I think that's a nice approach. Otherwise, I think the first makes the most sense.

@lifeiscontent
Copy link
Copy Markdown

lifeiscontent commented Nov 19, 2019

@jgodson it'd probably be a good idea to let people opt-in to this behavior before 2.0 then make it default later.

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.

4 participants