datastax / cql-proxy

A client-side CQL proxy/sidecar.
Apache License 2.0
172 stars 83 forks source link

Fix set keyspace result for python driver #127

Closed ikehara closed 3 months ago

ikehara commented 4 months ago

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.

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."

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.
absurdfarce commented 4 months ago

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!

ikehara commented 4 months ago

Thank you for the investigation.

While investigating this issue, I discovered another problem.

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

mpenick commented 4 months ago

I'm +1 this solution.

absurdfarce commented 3 months ago

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:

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.