Managing schema registry's schema references #50

FrancescoPessina · 2020-11-08T14:58:51Z

This PR enables Avro schema parsing with new schema registry's schema references feature, introducing a dedicated lookup function for references got from CSR.
Changes in this PR comes from the discussion of the deprecated #49 pull request.

cc @Strech

…eferences

Strech

Thanks a lot for your effort the code looks much cleaner now 👍🏼

In addition to specific points, I have a few recommendations about the code style:

Please don't prefix variables, we have a file context and it should cover that need
Please don't use 1-2 char variables, it makes code harder to read and it's acceptable in few cases when a shorter name will give some benefits.
Please run mix format to align the code with project formatting rules (they are pretty standard, except the line length)

Strech · 2020-11-10T13:18:35Z

lib/avrora/storage/registry.ex

@@ -33,7 +33,7 @@ defmodule Avrora.Storage.Registry do
         {:ok, id} <- Map.fetch(response, "id"),
         {:ok, version} <- Map.fetch(response, "version"),
         {:ok, schema} <- Map.fetch(response, "schema"),
-         {:ok, schema} <- Schema.parse(schema) do
+         {:ok, schema} <- Schema.parse(schema, csr_references_lookup_function(response)) do


Since this function is local and will be called on each reference we should apply a few adjustments:

Rename csr_references_lookup_function to make_references_lookup_fun because the scope of the whole module is CSR so no need to mention it in any name here

All the references must be prepared in advance otherwise (like in the current implementation) they will be extracted on each function call

As an idea:

Suggested change

{:ok, schema} <- Schema.parse(schema, csr_references_lookup_function(response)) do

{:ok, references} <- extract_references(response),

{:ok, schema} <- Schema.parse(schema, make_references_lookup_fun(references)) do

where references is a Map

Strech · 2020-11-10T13:31:49Z

lib/avrora/storage/registry.ex

@@ -93,10 +93,26 @@ defmodule Avrora.Storage.Registry do
  @spec configured?() :: true | false
  def configured?, do: !is_nil(registry_url())

+  defp csr_references_lookup_function(response) do


To comply with typespec of reference lookup function from Avrora.Schema we should always return a function with interface like

@type reference_lookup_fun :: (String.t() -> {:ok, String.t()} | {:error, term()})

which means for us that we can re-use the default lookup function Avrora.Schema.reference_lookup/1

Strech · 2020-11-10T13:35:10Z

lib/avrora/storage/registry.ex

+    case Map.fetch(response, "references") do
+      {:ok, csr_references} ->
+        references = Enum.reduce(csr_references, %{}, fn(cr, map) ->
+          {:ok, schema_map} = get(cr["subject"])


This will throw a matching error, but what if we can use case + throw to take control over it and return {:error, ...} instead. Also would be nice to cover it with a test of something like "reference can't be obtained"

Strech · 2020-11-10T13:44:33Z

lib/avrora/storage/registry.ex

+        fn(r) ->
+          json_schema = Map.get(references, r)
+          {:ok, json_schema}
+        end


Suggested change

fn(r) ->

json_schema = Map.get(references, r)

{:ok, json_schema}

end

{:ok, &Map.fetch(references, &1)}

@Strech thank you for your review, I'm a newbie in Elixir so any correction is very important for me.
I think I've applied all your corrections in last commit. Please take a look and if I missed something please let me know. Thanks.

…hema test

Strech

Thanks for the refactoring, let's improve the reference extraction and we will be good to go 🚀

Strech · 2020-11-12T08:52:48Z

lib/avrora/storage/registry.ex

+    end
+  end
+
+  defp make_references_lookup_fun(references) do


I think a general correction for the whole file - reference_lookup because it's a function to resolve 1 reference at a time. So better to name it

Strech · 2020-11-12T09:02:28Z

test/avrora/storage/registry_test.exs

+        {:error, subject_not_found_parsed_error()}
+      end)
+
+      assert catch_throw(Registry.get("io.confluent.Account") == :unknown_subject)


I think it was a bit of misunderstanding, this assertion should look like this

Suggested change

assert catch_throw(Registry.get("io.confluent.Account") == :unknown_subject)

assert Registry.get("io.confluent.Account") == {:error, :unknown_reference_subject}

Strech · 2020-11-12T09:03:04Z

lib/avrora/storage/registry.ex

+    end
+  end
+
+  defp get_reference_schema(map, subject) do


This method should be inlined (i.e removed) back to the extract_references/1

Strech · 2020-11-12T09:15:44Z

lib/avrora/storage/registry.ex

+  defp extract_references(response) do
+    case Map.fetch(response, "references") do
+      {:ok, references} ->
+        references_map =
+          Enum.reduce(references, %{}, fn cr, map ->
+            get_reference_schema(map, cr["subject"])
+          end)
+
+        {:ok, references_map}
+
+      :error ->
+        {:ok, :schema_without_references}
+    end
+  end


By using throw I mean that we need to exit from the inner loop with error, but not throw it to outside, throw it's just a control flow in this case.

I would like to suggest something like this

Suggested change

defp extract_references(response) do

case Map.fetch(response, "references") do

{:ok, references} ->

references_map =

Enum.reduce(references, %{}, fn cr, map ->

get_reference_schema(map, cr["subject"])

end)

{:ok, references_map}

:error ->

{:ok, :schema_without_references}

end

end

defp extract_references(response) do

references =

response

|> Map.get("references", [])

|> Enum.reduce(%{}, fn reference, memo ->

case get(reference["subject"]) do

{:ok, schema} -> Map.put(memo, schema.full_name, schema.json)

{:error, error} -> throw(error)

end

end)

{:ok, references}

catch

{:error, :unknown_subject} -> {:error, :unknown_reference_subject}

error -> {:error, error}

end

Strech · 2020-11-12T09:19:24Z

lib/avrora/storage/registry.ex

@@ -33,7 +33,8 @@ defmodule Avrora.Storage.Registry do
         {:ok, id} <- Map.fetch(response, "id"),
         {:ok, version} <- Map.fetch(response, "version"),
         {:ok, schema} <- Map.fetch(response, "schema"),
-         {:ok, schema} <- Schema.parse(schema) do
+         {:ok, references} <- extract_references(response),
+         {:ok, schema} <- Schema.parse(schema, make_references_lookup_fun(references)) do


Please rename make_references_lookup_fun to make_reference_lookup_fun (see below)

Strech · 2020-11-12T12:03:34Z

lib/avrora/storage/registry.ex

+  defp make_references_lookup_fun(references) do
+    case references do
+      :schema_without_references -> &Avrora.Schema.reference_lookup/1
+      _ -> &Map.fetch(references, &1)
+    end
+  end


Not sure why the credo silent about it, but for such a case if/else should be used

if references == :no_references, do: &Avrora.Schema.reference_lookup/1 else: &Map.fetch(references, &1)

And then we can return {:ok, %{}} in extract_references and use the same if/else

if references == %{}, do: &Avrora.Schema.reference_lookup/1 else: &Map.fetch(references, &1) end

and from here we can go to pattern matching

Suggested change

defp make_references_lookup_fun(references) do

case references do

:schema_without_references -> &Avrora.Schema.reference_lookup/1

_ -> &Map.fetch(references, &1)

end

end

def make_reference_lookup_function(%{}), do: &Avrora.Schema.reference_lookup/1

def make_reference_lookup_function(references), do: &Map.fetch(references, &1)

…n management

FrancescoPessina · 2020-11-14T14:16:00Z

@Strech I applied your advices. Thanks :)
I also added reference version in get call, see here:

https://github.com/FrancescoPessina/avrora/blob/e4963e1d49ac5d478d987d9b0476b389120d4df7/lib/avrora/storage/registry.ex#L103-L105

Strech · 2020-11-16T14:32:28Z

@FrancescoPessina Wanna say thanks for amazing contribution 🎉 . I will merge your PR, but the version release will be together with #47

If it will take too long (week+) for Klarna to release klarna/erlavro#104 then I would make preparations and release the next version without nil fix.

FrancescoPessina · 2020-11-16T14:33:58Z

@Strech thank you very much for your precise review :)
Ok, I'll wait the version release, no problem ;)

Thanks again

Strech · 2020-11-26T10:51:05Z

Hey, @FrancescoPessina a heads-up! The new 0.14 release was published on the Hex. Thanks a lot for your help, I appreciate it!

Adding dedicated lookup function to manage schema registry's schema r…

2e9938d

…eferences

FrancescoPessina mentioned this pull request Nov 8, 2020

Managing schema registry's schema references #49

Closed

Fixed mix credo issue replacing with clause with case

6fd72f7

FrancescoPessina force-pushed the feature/csr-schema-references branch from 78b8e3b to 6fd72f7 Compare November 9, 2020 08:06

Strech requested changes Nov 10, 2020

View reviewed changes

Refactored reference lookup function name; added missing reference sc…

b25f31a

…hema test

Strech requested changes Nov 12, 2020

View reviewed changes

Strech reviewed Nov 12, 2020

View reviewed changes

Refactoring schema registry reference lookup; adding reference versio…

e4963e1

…n management

FrancescoPessina force-pushed the feature/csr-schema-references branch from 89d412d to e4963e1 Compare November 14, 2020 14:11

Strech approved these changes Nov 16, 2020

View reviewed changes

Strech merged commit 951de80 into Strech:master Nov 16, 2020

Strech mentioned this pull request Nov 16, 2020

Implement support of Schema Registry's schema references #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Managing schema registry's schema references #50

Managing schema registry's schema references #50

FrancescoPessina commented Nov 8, 2020

Strech left a comment

Strech Nov 10, 2020

Strech Nov 10, 2020

Strech Nov 10, 2020 •

edited

Loading

Strech Nov 10, 2020

FrancescoPessina Nov 11, 2020

Strech left a comment

Strech Nov 12, 2020 •

edited

Loading

Strech Nov 12, 2020

Strech Nov 12, 2020

Strech Nov 12, 2020

Strech Nov 12, 2020

Strech Nov 12, 2020

FrancescoPessina commented Nov 14, 2020

Strech commented Nov 16, 2020 •

edited

Loading

FrancescoPessina commented Nov 16, 2020

Strech commented Nov 26, 2020

	{:ok, schema} <- Schema.parse(schema, csr_references_lookup_function(response)) do
	{:ok, references} <- extract_references(response),
	{:ok, schema} <- Schema.parse(schema, make_references_lookup_fun(references)) do

	assert catch_throw(Registry.get("io.confluent.Account") == :unknown_subject)
	assert Registry.get("io.confluent.Account") == {:error, :unknown_reference_subject}

Managing schema registry's schema references #50

Managing schema registry's schema references #50

Conversation

FrancescoPessina commented Nov 8, 2020

Strech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech left a comment

Choose a reason for hiding this comment

Strech Nov 12, 2020 • edited Loading

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

FrancescoPessina commented Nov 14, 2020

Strech commented Nov 16, 2020 • edited Loading

FrancescoPessina commented Nov 16, 2020

Strech commented Nov 26, 2020

Strech Nov 10, 2020 •

edited

Loading

Strech Nov 12, 2020 •

edited

Loading

Strech commented Nov 16, 2020 •

edited

Loading