-
Notifications
You must be signed in to change notification settings - Fork 89
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
Consider Base.url_(de/en)code64
with padding: false
for Global IDs
#85
Comments
Seems reasonable to me, @bruce thoughts? |
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. |
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 Now I assume rather than requiring the user to define a generator, absinthe will continue to provide a sane default, like the |
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. |
Thinking about implementation, I'm torn on where to place the configuration. Implementation ProposalNode ID Translator
ConfigurationA few of methods for consideration: In Schema module definitiondefmodule 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 configconfig Absinthe.Relay,
schema: [
MyApp.Schema: [
global_id_translator: MyApp.Schema.MyNodeIDTranslator,
],
] |
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 |
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 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 |
Totally agree. My |
My line of thinking was similar to @jparise's in respect to mix config. 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. |
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? |
I believe I do. Will be able to look at working on this in the next day or two. |
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. |
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 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. |
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.
The text was updated successfully, but these errors were encountered: