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

Change the default behavior of the encoder to use urlsafe versions #1698

Merged
merged 5 commits into from
Sep 10, 2018

Conversation

gfx
Copy link
Contributor

@gfx gfx commented Jul 24, 2018

I believe this is the better default because it is safely used as a pagination parameter of web apps (maybe like single-page applications).

Anyway, I define a custom encoder to use urlsafe versions in my project, but it's my pleasure if it is useful for your projects. What do you think of it?

In addition, I think lib/graphql/schema/unique_within_type.rb should use G::S::Base64Encoder, although the PR does not include this idea. How about it?

compatibility

  • .encode - The string changes
  • .decode - it can decodes what is encoded by Base64.strict_encode64

@rmosolgo
Copy link
Owner

🤔 Hmm, this issue has come up a few times, but I was always opposed because of compatibility issues.

But, it sounds like this decoder would be backwards-compatible, is that right? I mean, let's say someone updates their gem version. Would any of their API clients have trouble with outstanding cursors?

@gfx
Copy link
Contributor Author

gfx commented Jul 26, 2018

@rmosolgo

I think there's no trouble on updating a new gem that includes this PR.

This is because there are only / and + included in the original Base64 (see https://en.wikipedia.org/wiki/Base64#Base64_table ), and the urlsafe_decode64 replaces - and _ to / and + respectively, but it still able to handle / and + that the cursors encoded by Base64.strict_encode64 might include.

@rmosolgo
Copy link
Owner

rmosolgo commented Aug 1, 2018

Thanks for your response, I want to include this change in GraphQL-Ruby soon, maybe 1.9, let me think about it a bit more! (Actually I'm trying to migrate GitHub to urlsafe_encode64 right now!)

@rmosolgo
Copy link
Owner

rmosolgo commented Aug 7, 2018

I noticed an interesting issue when I deployed a similar change in github.com. We got some encoding issues, but here's what happened:

  • I updated the code to use urlsafe_{en|de}code64, knowing that it was backwards-compatible
  • I started a deploy
  • During the deploy, servers got the latest code and then restarted the app
  • But some apps started faster than others...
  • And in some cases, clients received new, url-safe cursors, but...
  • Then sent those new servers back to our website, and those requests were routed to old servers who hadn't restarted yet
  • Since they hadn't restarted yet, they tried to call .decode64 on url-safe cursors, which made a best-effort but incomplete conversion
  • Then, parsing the decoded payload content failed

Of course, those requests succeded on retry, because by that time, the server had properly restarted.

So, there's still a small compatibility issue here, but probably not worth worrying about. Just thought I'd share!

@gfx
Copy link
Contributor Author

gfx commented Aug 8, 2018

That is an interesting report! Thanks for sharing it. It means that before this PR, graphql-ruby must be able to decode url-safe base64 cursors for forward compatibilities.

gfx added a commit to gfx/graphql-ruby that referenced this pull request Aug 8, 2018
@rmosolgo rmosolgo added this to the 1.9.0 milestone Sep 10, 2018
@rmosolgo rmosolgo changed the base branch from master to 1.9-dev September 10, 2018 14:38
@rmosolgo
Copy link
Owner

I pushed a merge commit and changed the base to 1.9-dev, assuming the tests pass, I'll get this improvement to into 1.9.0 😄

@rmosolgo rmosolgo merged commit 18d5461 into rmosolgo:1.9-dev Sep 10, 2018
rmosolgo pushed a commit that referenced this pull request Sep 13, 2018
Change the default behavior of the encoder to use `urlsafe` versions
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