Implement overridable global ID translator #93

avitex · 2017-11-03T12:40:51Z

As per #85

TODO

Documentation
Tests

Merge master

avitex · 2017-11-03T13:07:50Z

LMKWYT.
Will write some documentation and a couple of tests this weekend.

avitex · 2017-11-21T01:50:44Z

lib/absinthe/relay/node.ex

@@ -200,6 +200,9 @@ defmodule Absinthe.Relay.Node do
    end
  end

+  @doc """
+  Similar to `from_global_id/2` but raises `RuntimeError` if fails to decode global ID.


@bruce Not sure if RuntimeError is correct in this context (same with to_global_id!/2). What do you think?

bruce

@avitex Really good work on this. There are a few changes here that I'd like you to look at, and if you have any questions please let me know.

We need to be really explicit about any backwards compatibility issues, so let's also make sure we chat about that before we merge this; we really should stay backwards-compatible until v1.5.

bruce · 2017-11-21T19:04:31Z

lib/absinthe/relay/node.ex

@@ -110,6 +113,8 @@ defmodule Absinthe.Relay.Node do

  require Logger

+  @type global_id_t :: binary


Let's make this @type global_id :: binary (we're moving away from the _t suffix in our types to better match wider community conventions).

bruce · 2017-11-21T19:15:25Z

lib/absinthe/relay/node.ex

+      {:ok, result} ->
+        result
+      {:error, err} ->
+        raise RuntimeError, message: err


Let's make a dedicated, specific error type for this in case anyone wants to catch it.

bruce · 2017-11-21T19:23:49Z

lib/absinthe/relay/node/id_translator/default.ex

@@ -0,0 +1,25 @@
+defmodule Absinthe.Relay.Node.IDTranslator.Default do


Let's be more explicit with this and call it Absinthe.Relay.Node.IDTranslator.Base64—the fact it's the default doesn't tell you what it does when you see it referenced in code/configuration.

bruce · 2017-11-21T19:31:38Z

lib/absinthe/relay/schema.ex

@@ -5,10 +5,26 @@ defmodule Absinthe.Relay.Schema do
  See `Absinthe.Relay`.
  """

-  defmacro __using__(opts) do
+  defmacro __using__ do


Not needed.

use Absinthe.Relay.Schema won't call this; use always calls __using__/1, even if no options are provided.

Reference: https://hexdocs.pm/elixir/Kernel.html#use/2

Learn something new every day, never knew this.

bruce · 2017-11-21T19:34:17Z

lib/absinthe/relay/schema.ex

      import_types Absinthe.Relay.Connection.Types
+
+      def __absinthe_relay_global_id_translator__ do
+        Keyword.get(unquote(opts), :global_id_translator, nil)


Third argument not necessary. Keyword.get/2 returns nil when not found.

Yeah, this is an old habit I have in being a bit too explicit

bruce · 2017-11-21T19:46:26Z

lib/absinthe/relay/node/id_translator.ex

+  end
+  ```
+  """
+


Callback docs?

bruce · 2017-11-21T19:48:02Z

lib/absinthe/relay/node.ex

+    |> apply(direction, args ++ [schema])
+  end
+
+  @non_relay_schema_error "Non relay schema provided"


Capitalize "Relay"

bruce · 2017-11-21T19:52:13Z

lib/absinthe/relay/node.ex

+
+  @non_relay_schema_error "Non relay schema provided"
+
+  defp global_id_translator(:env, schema) do


It might be nice to have this just be global_id_translator/1, and have the env vs schema vs default be an implementation concern of the function vs using a "magic atom" argument. Could you also make it def instead of defp for testing/debugging purposes?

With this in place, translate_global_id/3 could be simplified quite a bit.

bruce · 2017-11-21T19:52:53Z

lib/absinthe/relay/node/id_translator.ex

+defmodule Absinthe.Relay.Node.IDTranslator do
+  @moduledoc """
+  An ID translator handles encoding and decoding a global ID
+  used in a relay node.


Capitalize "Relay"

bruce · 2017-11-21T20:00:36Z

lib/absinthe/relay/node.ex

+    apply(Absinthe.Relay.Node.IDTranslator.Default, direction, args ++ [nil])
+  end
+  defp translate_global_id(schema, direction, args) do
+    (global_id_translator(:env, schema) || global_id_translator(:schema, schema) || Absinthe.Relay.Node.IDTranslator.Default)


I'm pretty sure that the environment setting should be the fallback for the explicit schema (use) setting, not the other way around.

avitex · 2017-11-22T05:39:26Z

In terms of backwards compatibility, I think a solution till v1.5 could be to make to_global_id/2 return nil on an error as it does now in v1.4.

This would mean to_global_id!/2 would then need to return nil early if source_id was nil and raise if to_global_id/2 returns nil (Following the behaviour of to_global_id(_node_type, nil, _schema)).

Thoughts?

bruce · 2017-12-05T04:50:21Z

Note that to_global_id(_node_type, nil, _schema) doesn't raise, it warns and returns an error tuple.

In general, we want to avoid raising during runtime, as it doesn't give us a good method of returning useful information to the API user (exceptions are something we try to reserve for mistakes by the schema designer).

If you can make this more backwards compatible for now, let's do it.

avitex · 2017-12-05T07:37:34Z

Not sure if I understand completely.

Currently in the wild:

to_global_id/2 returns binary | nil
from_global_id/2 returns {:ok, nil} | {:ok, %{type: atom, id: binary}} | {:error, binary}.

Perhaps you are talking about global_id_resolver/2 which warns via report_fetch_id_error/2 and returns a error tuple?

I understand the benefits on not raising on runtime, but to_global_id/2 currently does not return an error tuple, which was my reasoning behind providing to_global_id!/2 (a stepping stone for upgrading).

I can foresee to_global_id/2 returning error tuples on v1.5.

I'll make the changes I outlined in my previous comment and push them up for review.

bruce · 2017-12-05T08:32:38Z

Ah, yes, sorry. Apparently I misread. That's what I get for skimming through code on my phone.

Personally, I see very little benefit in using a ! version that raises, for the reasons I mentioned earlier. I think it's fine if we provide it, but our defaults should certainly more gracefully handle errors.

avitex · 2017-12-05T10:05:38Z

Yeah, I agree with you. I'll remove it for this PR but we could introduce it in v1.5 and remove in a later version. Only purpose it serves is for a quick upgrade to the next version.

avitex · 2018-03-08T23:43:17Z

Heya @bruce, I've removed the bang functions and fixed the conflicts. Let me know if there is anything else you'd like to discuss or change.

tlvenn · 2018-03-22T06:53:27Z

@bruce any chance to merge this soon ? Thanks in advance !

madebyherzblut · 2018-04-11T17:00:59Z

I would love to see this merged (and released) as well. We use hashids to obfusicate the ID and a global ID translator would remove a lot of code. Let me know if I can help in any way 😊

bruce · 2018-04-11T17:43:25Z

Sorry folks, we're finally going to do some passes on PRs (we've been crushed with other stuff/exhausted) and will look at getting this in very soon.

bruce · 2018-04-12T18:34:31Z

This will ship in the next release (bundled with some other changes); will try to get it out today/tomorrow.

avitex added 5 commits November 3, 2017 17:19

Merge pull request #1 from absinthe-graphql/master

785352b

Merge master

Implement global_id_translator

19cedce

Fix environment config

b7d9888

Document default implementation

137fd6f

Provide convenience wrapper functions

ab5421a

avitex added 4 commits November 9, 2017 22:50

Document behaviour

0cf86f2

Piggyback off parse_ids test

f1e08e4

Improve typespecs and backwards compatibility

dc7d3f5

Document from_global_id!/2 and to_global_id!/3

7ee9b4e

avitex commented Nov 21, 2017

View reviewed changes

avitex added 2 commits November 21, 2017 13:46

Document configuration methods

b642193

Fix grammar

d155cf8

bruce suggested changes Nov 21, 2017

View reviewed changes

Refactor ID Translator

9f0bef7

bruce self-assigned this Nov 22, 2017

Backwards compatibility for v1.4

d7a0a93

avitex added 2 commits March 9, 2018 09:58

Remove to_global_id!/2 and from_global_id!/2

1169387

Merge upstream and fix conflict

ab0ca11

bruce merged commit ff4799a into absinthe-graphql:master Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement overridable global ID translator #93

Implement overridable global ID translator #93

avitex commented Nov 3, 2017 •

edited

Loading

avitex commented Nov 3, 2017

avitex Nov 21, 2017 •

edited

Loading

bruce left a comment

bruce Nov 21, 2017

bruce Nov 21, 2017

bruce Nov 21, 2017

bruce Nov 21, 2017

avitex Nov 22, 2017

bruce Nov 21, 2017

avitex Nov 22, 2017

bruce Nov 21, 2017

bruce Nov 21, 2017

bruce Nov 21, 2017

bruce Nov 21, 2017

bruce Nov 21, 2017

avitex commented Nov 22, 2017

bruce commented Dec 5, 2017

avitex commented Dec 5, 2017

bruce commented Dec 5, 2017

avitex commented Dec 5, 2017

avitex commented Mar 8, 2018

tlvenn commented Mar 22, 2018

madebyherzblut commented Apr 11, 2018

bruce commented Apr 11, 2018

bruce commented Apr 12, 2018

		@@ -110,6 +113,8 @@ defmodule Absinthe.Relay.Node do

		require Logger

		@type global_id_t :: binary

		@@ -0,0 +1,25 @@
		defmodule Absinthe.Relay.Node.IDTranslator.Default do


		@non_relay_schema_error "Non relay schema provided"

		defp global_id_translator(:env, schema) do

Implement overridable global ID translator #93

Implement overridable global ID translator #93

Conversation

avitex commented Nov 3, 2017 • edited Loading

TODO

avitex commented Nov 3, 2017

avitex Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

bruce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avitex commented Nov 22, 2017

bruce commented Dec 5, 2017

avitex commented Dec 5, 2017

bruce commented Dec 5, 2017

avitex commented Dec 5, 2017

avitex commented Mar 8, 2018

tlvenn commented Mar 22, 2018

madebyherzblut commented Apr 11, 2018

bruce commented Apr 11, 2018

bruce commented Apr 12, 2018

avitex commented Nov 3, 2017 •

edited

Loading

avitex Nov 21, 2017 •

edited

Loading