Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix set keyspace result for python driver #127

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

ikehara
Copy link
Contributor

@ikehara ikehara commented Jun 22, 2024

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.

* modify identifer.go to add ID method

* modify lexer_test.go to add TestLexerIdentifiers test

* mofidy proxy.go to fix interceptSystemQuery method because SetKeyspaceResult should be a real keyspace name

* fix quoted name of identifier
@absurdfarce
Copy link
Collaborator

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
Copy link
Contributor Author

ikehara commented Jun 29, 2024

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.

@@ -816,7 +816,8 @@ func (c *client) interceptSystemQuery(hdr *frame.Header, stmt interface{}) {
c.send(hdr, &message.ServerError{ErrorMessage: "Proxy unable to create new session for keyspace"})
} else {
c.keyspace = s.Keyspace
c.send(hdr, &message.SetKeyspaceResult{Keyspace: s.Keyspace})
ks := parser.IdentifierFromString(s.Keyspace)
Copy link
Contributor

@mpenick mpenick Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a version of the proxy we're using internally. This issue is fixed using this patch:

diff --git a/parser/parse_select.go b/parser/parse_select.go
index 1447de1..6aa2e33 100644
--- a/parser/parse_select.go
+++ b/parser/parse_select.go
@@ -69,7 +69,7 @@ func isHandledUseStmt(l *lexer) (handled bool, stmt Statement, err error) {
 	if tkIdentifier != t {
 		return false, nil, errors.New("expected identifier after 'USE' in use statement")
 	}
-	return true, &UseStatement{Keyspace: l.identifierStr()}, nil
+	return true, &UseStatement{Keyspace: l.identifier().id}, nil
 }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with the handling of double quotes it would look something like:

diff --git a/parser/parse_select.go b/parser/parse_select.go
index 1447de1..cee7815 100644
--- a/parser/parse_select.go
+++ b/parser/parse_select.go
@@ -69,7 +69,7 @@ func isHandledUseStmt(l *lexer) (handled bool, stmt Statement, err error) {
 	if tkIdentifier != t {
 		return false, nil, errors.New("expected identifier after 'USE' in use statement")
 	}
-	return true, &UseStatement{Keyspace: l.identifierStr()}, nil
+	return true, &UseStatement{Keyspace: l.identifier().ID()}, nil
 }

@mpenick
Copy link
Contributor

mpenick commented Jul 2, 2024

I'm +1 this solution.

@absurdfarce
Copy link
Collaborator

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 absurdfarce merged commit 0dea6ff into datastax:main Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants