Make the connection aware of the current keyspace #180
Conversation
| else | ||
| %_{} = response -> |
There was a problem hiding this comment.
@whatyouhide here I was not sure if I have to explicitly list all the possible structs that can be returned by decode_response, like
%kind{} when kind in [...]let me know if you prefer it that way or, option 3, if I should skip the last line in the with statement and do something like
case state.protocol_module.decode_response(frame, query, options) do
%SetKeyspace{keyspace: keyspace} ->
{:ok, query, response, %{state | current_keyspace: keyspace}}
response ->
{:ok, query, response, state}
end
|
@lexmag hey! Could you please take a look at this when you can, since you're more familiar with prepared queries than I am :) I'll be able to look at this late next week 😞 |
|
@lexmag @whatyouhide any update on this? Also, I think we're going to win a prize for the longest running CI in Github's history since it's |
Ahah, Travis dropped the ball a bit 😄 I will not be able to review this anytime soon, swamped with work and other commitments, sorry 😞 Will try to get to it as soon as possible! |
|
Hey @whatyouhide! Since I saw some activity on the repo (and the removal of Travis which was cursing this PR) I rebased this on the current I checked with the test and this is still relevant (i.e. the original bug is still there). The failing test in the CI is a result of #211. Let me know if I need to change something. |
There was a problem hiding this comment.
Hey @rbino, yes indeed, I am finally giving some love to Xandra.
So, as I understand your changes revolve around decoding the responses from C* in the connection module and not in the various "query" modules (Xandra.Batch, ...). I think this still happens in the caller and not in the connection process, so that should be fine.
| module_suffix = | ||
| inspect(__MODULE__) | ||
| |> String.replace(".", "") | ||
| |> String.downcase() | ||
|
|
||
| keyspace = "xandra_test_" <> module_suffix |
There was a problem hiding this comment.
Why is this necessary? Can't we compare with String.downcase/1 from the query result in the new tests?
There was a problem hiding this comment.
@whatyouhide as I explained in the commit message, I changed the test helper because the keyspace actually gets created lowercased (see https://docs.datastax.com/en/cql-oss/3.x/cql/cql_reference/ucase-lcase_r.html). The other tests didn't care because the name just gets passed as is and is translated by the DB automatically, but matching explicitly on the returned keyspace name revealed the inconsistency.
Of course I can just change this just in my test, I just thought that it was better to fix the inconsistency at the source so that tests can use the correct created keyspace name, let me know what I should do.
The keyspace that was created for the tests was effectively always created as lowercase (see https://docs.datastax.com/en/cql-oss/3.x/cql/cql_reference/ucase-lcase_r.html). This does not matter when using a USE statement since that gets lowercased too on the Cassandra side but it matters when explicitly matching on the returned keyspace in the SetKeyspace struct.
Use the keyspace information to add it to `%Prepared{}` structs so that it can
be used together with the statement as key in the prepared cache. This avoids
erroneously using queries that were prepared against another keyspace.
To do so, response decoding has been moved inside the connection, akin to what
was already done in `handle_prepare` to populate the prepared cache. The decode
function in query structs is now simply an identity since the result gets there
already decoded.
Fix whatyouhide#179
|
This is fantastic, thank you so much @rbino. 💟 |
Use the keyspace information to add it to
%Prepared{}structs so that it can be used together with the statement as key in the prepared cache. This avoids erroneously using queries that were prepared against another keyspace.To do so, response decoding has been moved inside the connection, akin to what was already done in
handle_prepareto populate the prepared cache. The decode function in query structs is now simply an identity since the result gets there already decoded.This also includes a minor fixup in the keyspace returned by
setup_allbyIntegrationCase.Fix #179