refactor(pairing): port queries to xandra #834
Conversation
Codecov Report
@@ Coverage Diff @@
## master #834 +/- ##
==========================================
+ Coverage 67.42% 75.23% +7.80%
==========================================
Files 264 139 -125
Lines 6429 2588 -3841
==========================================
- Hits 4335 1947 -2388
+ Misses 2094 641 -1453
|
|
|
||
| defp use_realm(conn, realm) when is_binary(realm) do | ||
| with true <- Realm.valid_name?(realm), | ||
| {:ok, %Xandra.SetKeyspace{}} <- Xandra.execute(conn, "USE #{realm}") do |
There was a problem hiding this comment.
For c42c32a I have opted for switching keyspace first and running the query second.
This is in contrast with #771 where we're currently using a query parameter and then manually replacing the string
While it is of course possible to do that here as well inside the prepare_query/3 function, I've found it overall cleaner, especially for error handling and query definition, to have the separate USE statement.
I'd like to discuss this as I think it is important to have coherent code in all our PRs
There was a problem hiding this comment.
The reason we did it with string interpolation is that there was an issue with how Xandra handled USE queries in older versions (see whatyouhide/xandra#180).
The PR took some time to be merged so we kept on using string interpolation, but now that it's upstreamed we should be fine using USE queries.
noaccOS
force-pushed
the
pairing-xandra
branch
3 times, most recently
from
2e924f9
to
7399bb5
Compare
| @@ -65,17 +65,21 @@ defmodule Astarte.Pairing.Config do | |||
| Returns the cassandra node configuration | |||
| """ | |||
| @spec cassandra_node!() :: {String.t(), integer()} | |||
| def cassandra_node!, do: Enum.random(cqex_nodes!()) | |||
| def cassandra_node!, do: Enum.random(xandra_nodes!()) | |||
There was a problem hiding this comment.
This function should be unused now, as it was required by CQEx but not by Xandra. It can be removed
| @typedoc """ | ||
| A string representing the current context, to include in error messages. | ||
| Not included if nil. | ||
| Default: nil | ||
| """ | ||
| @type custom_context() :: String.t() | nil | ||
|
|
||
| @typedoc """ | ||
| The type to cast the result in. | ||
| Unless otherwise stated, all results are returned in an {:ok, value} tuple. | ||
| - `:page`: default xandra return type | ||
| - `:list`: a list | ||
| - `{:first, default}`: only return the first element, `default` if empty | ||
| - `:first`: shorthand for `{:first, nil}` | ||
| - `{:first!, error}`: like `{:first, default}`, but returns `{:error, error}` if empty | ||
| - `:first!`: shorthand for `{:first!, :not_found}` | ||
| """ | ||
| @type custom_result() :: :page | :list | :first | :first! | {:first, any()} | {:first!, any()} | ||
|
|
||
| @type custom_opt() :: {:context, custom_context()} | {:result, custom_result()} | ||
| @type xandra_opt() :: {atom(), any()} | ||
|
|
||
| @type query_opt() :: custom_opt() | xandra_opt() |
There was a problem hiding this comment.
That's a good effort to get to a safer typing of queries. However, I'm not sure that this would be the best place: this is something I would expect to find in a library (e.g. Astarte Data Access) rather than in an application module. For example, the custom_result() type is generic enough not to be related just to the queries in this module.
Nonetheless, this approach is interesting, so I say we should discuss it further.
| @type custom_context() :: String.t() | nil | ||
|
|
||
| @typedoc """ | ||
| The type to cast the result in. |
There was a problem hiding this comment.
The fact that the type is casted is an implementation detail that could be hidden
| end | ||
|
|
||
| defp database_error_message(%Xandra.Error{message: message, reason: reason}, nil = _context) do | ||
| %{message: "Database error #{reason}: #{message}", tag: "db_error"} |
There was a problem hiding this comment.
Since Xandra errors implement the Exception behaviour, you could use Exception.message/1 rather than manually getting into the struct and retrieving message and reason
Signed-off-by: Francesco Noacco <[email protected]>
use xandra in place of cqex for all queries to the cluster. feat: expose cluster name as an env var internally to the config fix: the tests now wait for the cluster to be connected fix: remove :autodiscovery key from xandra_options as it is deprecated chore: update xandra to v0.17.0 Signed-off-by: Francesco Noacco <[email protected]>
noaccOS
force-pushed
the
pairing-xandra
branch
from
7399bb5
to
06b2a2a
Compare
use xandra in place of cqex for all queries to the cluster.
feat: expose custom query API
feat: expose cluster name as an env var internally to the config
fix: the tests now wait for the cluster to be connected
fix: remove :autodiscovery key from xandra_options as it is deprecated
chore: update xandra to v0.17.0
closes #268