absinthe-graphql / absinthe_relay

Absinthe support for the Relay framework
MIT License
182 stars 88 forks source link

Consider `Base.url_(de/en)code64` with `padding: false` for Global IDs #85

Open avitex opened 7 years ago

avitex commented 7 years ago

Using IDs in URLs is a common feature with sites, so ensuring they are URL safe is fairly important.

The standard library provides url_encode64/2 and url_decode64/2 with this case in mind.

I would also suggest that padding be disabled, as its use in this context does not make much sense. It adds bytes on the line and does not look very tasteful in a URL.

What are your thoughts? Happy to craft a PR if acceptable.

benwilson512 commented 7 years ago

Seems reasonable to me, @bruce thoughts?

bruce commented 7 years ago

This should mirror Relay’s implementation by default, so if Relay’s Node implementation uses padding, this will need to be a be a configurable option rather than a hard coded one.

Otherwise, go for it.

avitex commented 7 years ago

After sleeping on the idea, my personal opinion is that the method of generating global IDs should be overridable.

Node IDs don't have a specification in terms of how you generate them, only that they be treated as opaque keys on the client side and be globally unique. Base64 is the general convention used in examples, as it reminds developers that that the key should be treated as such, in a simple way.

For inspiration in the wild graphql-ruby requires configuring the global id generator, and provides a simple base64 generator as found in relay examples for getting off the ground quickly.

Now I assume rather than requiring the user to define a generator, absinthe will continue to provide a sane default, like the Base.(en/de)code64 in use now. However, with a overridable method of generation (relay does not care), developers can implement it as they wish. A example use case of this would be encryption to hide the database ID.

bruce commented 7 years ago

This has long been an idea, and we’d love to see it.

And agreed on what’s specified; I just want to make sure the “out of the box” experience matches what JavaScript developers coming from a isomorphic JS experience will expect, as that is a primary adoption path at the moment.

avitex commented 7 years ago

Thinking about implementation, I'm torn on where to place the configuration. Ideally this should be a per schema option and not to the degree where you have to reference the ID translator everywhere.

Implementation Proposal

Node ID Translator

Absinthe.Relay.Node.IDTranslator defines a behavior with the methods from_global_id/? and to_global_id/?.

Absinthe.Relay.Node.IDTranslator.Base64 defines a default implementation using base64.

Configuration

A few of methods for consideration:

In Schema module definition

defmodule MyApp.Schema do
  use Absinthe.Schema
  # This would be my preferred method of configuration
  use Absinthe.Relay.Schema, [
    global_id_translator: MyApp.Schema.MyNodeIDTranslator,
  ]

  # ...

  # Macro based
  global_id_translator MyApp.Schema.MyNodeIDTranslator
end

Using mix config

config Absinthe.Relay,
  schema: [
    MyApp.Schema: [
      global_id_translator: MyApp.Schema.MyNodeIDTranslator,
    ],
  ]
jparise commented 7 years ago

I think I'd go the Schema route. This level of configuration is more tightly coupled to the representation of the API than, say, a pluggable JSON module, which is an implementation detail. The latter is what I tend to customize via application configuration.

And if someone really needed to configure global_id_translator on a per-environment basis, that would still be possible using Application.get_env/3.

bruce commented 7 years ago

And if someone really needed to configure global_id_translator on a per-environment basis, that would still be possible using Application.get_env/3.

My understanding is that this wouldn't work if they were packaging this up at release, though, since this would be a compile-time configuration (ie, if the get_env/3 was used right there in option passed to use).

I don't see any reason not to support both methods here, I can see clear purposes and configuration patterns for each (although I don't see a need for the :schema namespacing in the config setting).

jparise commented 7 years ago

Totally agree. My get_env/3 comment was more about customizing the innards of the translator implementation, not making the translator swappable, but I should have been clearer.

avitex commented 7 years ago

My line of thinking was similar to @jparise's in respect to mix config. If the developer wished to configure on a per environment basis they could themselves with get_env/3.

An additional point not I did not initially mention was that customization in absinthe_relay so far is inlined. Perhaps an apples and oranges comparison but an example of this is the classic/modern switch in the 1.4 branch). Following that pattern that made sense to me. However it could be debated that this would have value in mix config too, as a developer may wish to use Base64 encoding on their local platform for development and an encryption based method for production.

I'm happy to implement both and can see the potential value, though I personally favor the inline method and agree with @jparise's sentiments.

I would assume if both were to be implemented, mix would take precedence over inline.

bruce commented 6 years ago

I'd like to see both supported if you'd still like to move forward on this @avitex. I think it's best to continue the discussion around code vs in the abstract. Do you have enough to take a stab at it?

avitex commented 6 years ago

I believe I do. Will be able to look at working on this in the next day or two.

tlvenn commented 6 years ago

Just wanted to share how I hope to leverage this feature. In my system, all IDs are unique by default however 99% of them are 64bits integers (snowflake variant) which are simply casted to strings.

This is not much of a concern as far as transport is concerned although it could help a tiny bit but the client is building a normalised cache out of those ids and they could be packed using an higher base before being sent to the client so the memory footprint of that cache is reduced.

This is not really a concern for a desktop app but on mobile it matters.

halostatue commented 4 years ago

A bump on this one. I hit something today that I’d managed to avoid for almost three years…I’m generating Base64 URL encoding on the web side (because I do use these values in URLs) and while most of this is just fine…I hit a problem on the Elixir side that would be simplified by specifying padding: false in the default Relay ID decoder. Most of the URL unsafe strings currently come when you use values outside of the printable strings, I think (at least I’ve never seen one in three years).

iex> z = "ChargeInvoice:c524ce41-5987-480f-84c8-f3103f069b04"
iex> Base.encode64(z)
"Q2hhcmdlSW52b2ljZTpjNTI0Y2U0MS01OTg3LTQ4MGYtODRjOC1mMzEwM2YwNjliMDQ="
iex> z |> Base.encode64 |> Base.decode64 == z
true
iex> z |> Base.encode64 |> String.replace(~r/=+$/, "") |> Base.decode64
:error
iex> z |> Base.encode64 |> String.replace(~r/=+$/, "") |> Base.decode64!(padding: false) == z
true

I’m going to work around this on my side with an overridable ID translator, but this should be done to prevent explosions.