Add translation function from realm to keyspace #105

eddbbt · 2024-03-06T12:12:32Z

Added a function to support new database keyspaces separation.
The function translate from a realm name to a keyspace name, derived from realm + astarte_instance_id (optional parameter).

This edit has became necessary before an extensive rewriting of the main astarte repo, in order to add the database keyspaces separation feature.

New tests have been written to cover main and side cases.

After internal discussion, this feature does not use persistent_term anymore

resolves astarte-platform/astarte#924

codecov · 2024-03-06T12:16:03Z

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 75.11%. Comparing base (3a4fe8d) to head (d5a284a).
Report is 1 commits behind head on master.

Files	Patch %	Lines
lib/astarte_core/cql_utils.ex	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   75.08%   75.11%   +0.02%     
==========================================
  Files          50       50              
  Lines         839      844       +5     
==========================================
+ Hits          630      634       +4     
- Misses        209      210       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Annopaolo · 2024-03-06T14:20:48Z

lib/astarte_core/cql_utils.ex

+  @spec get_from_realm!(any()) :: any()
+  def get_from_realm!("") do
+    raise "EmptyRealm"
+  end
+
+  def get_from_realm!(realm_name) when is_binary(realm_name) do


I would go with

Suggested change

@spec get_from_realm!(any()) :: any()

def get_from_realm!("") do

raise "EmptyRealm"

end

def get_from_realm!(realm_name) when is_binary(realm_name) do

@spec realm_name_to_keyspace_name(nonempty_binary()) :: nonempty_binary()

def realm_name_to_keyspace_name(realm_name) do

So that we keep consistency in naming and behavior with other functions in the module.

Another option, since the instance id will be known by the user of the library, could be

@spec realm_name_to_keyspace_name(nonempty_binary()) :: nonempty_binary() @spec realm_name_to_keyspace_name(nonempty_binary(), nonempty_binary()) :: nonempty_binary() def realm_name_to_keyspace_name(realm_name, astarte_instance_id \\ "default") do #...

Annopaolo · 2024-03-06T14:22:59Z

lib/astarte_core/cql_utils.ex

+  end
+
+  def get_from_realm!(realm_name) when is_binary(realm_name) do
+    customer_keyspace = :persistent_term.get("instance_id")


This should be either retrieved via config (see e.g. Astarte Data Access envs) or passed as an argument to the function

actually, direct access of persistent_term or whatever env value from main application managed by Skogsra will not work, so this value needs to be passed as an argument

Annopaolo

Code is good! Just some details in tests might be improved

Annopaolo · 2024-03-18T17:17:25Z

test/astarte_core/cql_utils_test.exs

+    assert Realm.valid_name?(CQLUtils.realm_name_to_keyspace_name("example", "atestinstance")) ==
+             true
+
+    :persistent_term.erase("example")


The test is tasked just with the creation of the keyspace name and its storage, so the call to :persistent_term.erase/1 would be better inside the on_exit callback.
You could also add a line with this assertion:

assert :persistent_term.get("example") = "atestinstanceexample"

This also holds for the other new tests

Annopaolo · 2024-03-18T17:19:55Z

test/astarte_core/cql_utils_test.exs

+    :persistent_term.erase("example")
+  end
+
+  test "realm name gets encoded, persistent_term exists" do


What you want to test here is probably that the keyspace name is generated even if the realm name is longer than 47 characters, so I suggest rephrasing the test as

Suggested change

test "realm name gets encoded, persistent_term exists" do

test "realm name is > 47 characters long, persistent_term exists" do

This implies that the previous one must also be renamed
test "realm name is <= 47 characters long, [..]"

Annopaolo · 2024-03-18T17:21:03Z

test/astarte_core/cql_utils_test.exs

+    :persistent_term.erase("example")
+  end
+
+  test "realm name is not empty, persistent_term does not exists" do


Does not exists?

rbino · 2024-03-20T11:32:46Z

lib/astarte_core/cql_utils.ex

@@ -167,4 +167,35 @@ defmodule Astarte.Core.CQLUtils do

    eid
  end
+
+  @spec realm_name_to_keyspace_name(nonempty_binary(), nonempty_binary()) :: nonempty_binary()
+  def realm_name_to_keyspace_name(realm_name, astarte_instance_id \\ "default")


The default astarte_instance_id should be an empty string, otherwise this would be a breaking change.

rbino · 2024-03-20T11:36:53Z

lib/astarte_core/cql_utils.ex

+  @spec realm_name_to_keyspace_name(nonempty_binary(), nonempty_binary()) :: nonempty_binary()
+  def realm_name_to_keyspace_name(realm_name, astarte_instance_id \\ "default")
+      when is_binary(realm_name) do
+    case :persistent_term.get(realm_name, nil) do


https://www.erlang.org/doc/man/persistent_term#best-practices-for-using-persistent-terms

You should not use the bare realm as key since that might cause conflicts given that persistent terms are global. You should at least namespace it with an atom, e.g. {:astarte_realm_to_keyspace, realm_name}

rbino · 2024-03-20T11:40:33Z

lib/astarte_core/cql_utils.ex

+        if String.length(astarte_instance_id <> realm_name) < 48 do
+          keyspace_name =
+            (astarte_instance_id <> realm_name)
+            |> String.replace("-", "")
+            |> String.replace("_", "")
+            |> String.downcase()
+
+          :persistent_term.put(realm_name, keyspace_name)
+          keyspace_name
+        else
+          keyspace_name =
+            Base.url_encode64(astarte_instance_id <> realm_name, padding: false)
+            |> String.replace("-", "")
+            |> String.replace("_", "")
+            |> String.slice(0..47)
+            |> String.downcase()
+
+          :persistent_term.put(realm_name, keyspace_name)
+          keyspace_name


I'd isolate the pure function from the side effects, i.e. realm_name_to_keyspace_name/2 just transforms the realm name in a keyspace name, and then persistent_realm_name_to_keyspace_name/2 gets it from persistent term (calculating it using the previous function if not found).
This also allows you to unit test the logic of the pure function without worrying about persistent terms.

(the names are a suggestion, I'm open to other proposals too)

@rbino the idea behind the current impl is that it's faster to retrieve an already encoded realm name from persistent_terms rather than re-encode each time it is accessed.
Probabily it's better to just rename the function in persistent_realm_name_to_keyspace_name and create a private one for the encoding? since it's useless to encode without check persistent_term first outside of this module

@rbino something like this

Currently Astarte instances are not allowd to share the same database due to a conflict in namespaces resolution (eg. two realms with same name in different astarte instances now share the same kespace, that's not good) This commit introduce univoque mapping between the pair <Astarte instance id, realm name> and keyspace name (see also astarte-platform/astarte_core#105), this way even with the same name, realms do not share the same keyspace. Also, the env var `ASTARTE_INSTANCE_ID`, defaulting to `""` has been added to maintain backward compatibility. Trivially change all queries to the database to reflect this change, both in `lib` and `tests`. Signed-off-by: Eddy Babetto <eddy.babetto@secomind.com>

eddbbt force-pushed the keyspace_feature branch from 6da0010 to 8a4529b Compare March 6, 2024 12:15

eddbbt force-pushed the keyspace_feature branch from 8a4529b to 7a72b1d Compare March 6, 2024 12:17

eddbbt marked this pull request as ready for review March 6, 2024 12:25

eddbbt force-pushed the keyspace_feature branch from 7a72b1d to 2a097e0 Compare March 6, 2024 13:51

Annopaolo requested changes Mar 6, 2024

View reviewed changes

eddbbt force-pushed the keyspace_feature branch 2 times, most recently from 25f1294 to 2d7dcf6 Compare March 6, 2024 14:42

eddbbt requested a review from Annopaolo March 6, 2024 14:43

eddbbt force-pushed the keyspace_feature branch 2 times, most recently from e7a52d7 to f75b6fb Compare March 6, 2024 16:29

Annopaolo approved these changes Mar 18, 2024

View reviewed changes

eddbbt changed the title ~~Add Keyspace retrieve and save~~ Function for translate from realm name to keyspace name Mar 18, 2024

eddbbt requested a review from Annopaolo March 18, 2024 13:50

eddbbt changed the title ~~Function for translate from realm name to keyspace name~~ Add translation function from realm to keyspace Mar 18, 2024

eddbbt mentioned this pull request Mar 18, 2024

Queries edit for keyspace separation feature astarte-platform/astarte_vmq_plugin#85

Merged

Annopaolo requested changes Mar 18, 2024

View reviewed changes

eddbbt mentioned this pull request Mar 19, 2024

Edit Astarte keyspaces astarte-platform/astarte#909

Merged

rbino requested changes Mar 20, 2024

View reviewed changes

shinnokdisengir self-requested a review March 20, 2024 13:02

eddbbt force-pushed the keyspace_feature branch from 32b2cb3 to 097d976 Compare March 20, 2024 13:17

eddbbt requested a review from rbino March 20, 2024 13:19

eddbbt force-pushed the keyspace_feature branch 3 times, most recently from 7940d45 to 936425d Compare March 20, 2024 13:35

eddbbt changed the title ~~Add translation function from realm to keyspace~~ [WIP] Add translation function from realm to keyspace Mar 20, 2024

eddbbt force-pushed the keyspace_feature branch from 936425d to bbf2731 Compare March 25, 2024 13:35

eddbbt changed the title ~~[WIP] Add translation function from realm to keyspace~~ Add translation function from realm to keyspace Mar 25, 2024

eddbbt requested a review from Annopaolo March 25, 2024 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add translation function from realm to keyspace #105

Add translation function from realm to keyspace #105

eddbbt commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 6, 2024 •

edited

Loading

Annopaolo Mar 6, 2024

Annopaolo Mar 6, 2024

eddbbt Mar 6, 2024 •

edited

Loading

Annopaolo left a comment

Annopaolo Mar 18, 2024

Annopaolo Mar 18, 2024

Annopaolo Mar 18, 2024

shinnokdisengir Mar 20, 2024

Annopaolo Mar 18, 2024

rbino Mar 20, 2024

rbino Mar 20, 2024

rbino Mar 20, 2024

rbino Mar 20, 2024

eddbbt Mar 20, 2024

eddbbt Mar 20, 2024

	test "realm name gets encoded, persistent_term exists" do
	test "realm name is > 47 characters long, persistent_term exists" do

Add translation function from realm to keyspace #105

Add translation function from realm to keyspace #105

Conversation

eddbbt commented Mar 6, 2024 • edited Loading

codecov bot commented Mar 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddbbt Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Annopaolo 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

eddbbt commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 6, 2024 •

edited

Loading

eddbbt Mar 6, 2024 •

edited

Loading