cql-proxy icon indicating copy to clipboard operation
cql-proxy copied to clipboard

Fix set keyspace result for python driver

Open ikehara opened this issue 1 year ago • 3 comments

This pull request fixes the issue where the SetKeyspaceResult in the response message of the USE statement does not reflect the actual keyspace name.

Previously, when using the USE statement with a quoted name in cqlsh, the behavior was as follows:

  1. cqlsh sends USE "ks"
  2. SetKeyspaceResult "\"ks\"" is returned
  3. cqlsh sends USE ""ks""

The Cassandra session is composed of multiple connections, and when the USE statement succeeds on one connection, it updates the current keyspace for all other connections.

  • https://github.com/datastax/python-driver/blob/3.29.1/cassandra/cluster.py#L4642

I also found that the escape character of quoted name is different from the CQL grammar. It says "any character where " can appear if doubled."

  • https://cassandra.apache.org/doc/stable/cassandra/cql/definitions.html#identifiers

Changes

This pull request includes the following changes:

  1. Modify identifer.go to add ID method

    • Added a new method ID to the identifer.go file to enhance identifier handling.
  2. Modify lexer_test.go to add TestLexerIdentifiers test

    • Introduced a new test case TestLexerIdentifiers in lexer_test.go to ensure the lexer correctly identifies and processes identifiers.
  3. Modify proxy.go to fix interceptSystemQuery method

    • Fixed the interceptSystemQuery method in proxy.go to ensure SetKeyspaceResult uses a real keyspace name instead of quoted one.
  4. Fix quoted name of identifier

    • Corrected the quoted name of the identifier to ensure proper syntax and functionality.

ikehara avatar Jun 22 '24 13:06 ikehara

Thanks for the PR @ikehara! There's definitely a problem here but I have a different take on the root cause of the issue. I'm going to create a new issue which will include my analysis of what I think is going on here; once that's done we can discuss how to proceed.

Thanks again!

absurdfarce avatar Jun 25 '24 04:06 absurdfarce

Thank you for the investigation.

While investigating this issue, I discovered another problem.

  • https://github.com/datastax/cql-proxy/pull/130

I would appreciate it if you could take a look at this one as well.

ikehara avatar Jun 29 '24 09:06 ikehara

I'm +1 this solution.

mpenick avatar Jul 02 '24 15:07 mpenick

So I didn't articulate this clearly enough but my original concern here was some variation of the following; there are actually two issues being addressed by this PR:

  • cql-proxy wasn't removing quotes around keyspace responses when sending set keyspace response messages
  • The parser wasn't correctly handling some of the quoting cases for CQL statements

They're both legit bugs, but only one really relates to the problem at hand. My hope was to detangle those issues so that we could get a clear fix for each of them. After talking about it for a while it's become increasingly clear that this isn't essential, and I agree the approach here solves the problem as well.

absurdfarce avatar Jul 29 '24 15:07 absurdfarce