Skip to content

Conversation

@derek9gag
Copy link

@derek9gag derek9gag commented Feb 10, 2020

The USE <keyspace> statements are inconsistently quoted in different places. It is impossible to use a quoted keyspace (for example keyspace with upper-case characters, see CPP-747).

This PR fixes a regression from v2.9.0. Keyspaces with uppercases likely never worked since v2.10.0 (9292b53).

This also resolves PHP-224.

See these lines:

@derek9gag derek9gag requested a review from mpenick February 10, 2020 08:09
This fixes a regession since v2.9, in which the driver always fail to connect to quoted keyspaces. Keyspaces with uppercase characters must be quoted.
Double-quotes around keyspace name is valid CQL, and the quote character should not be considered in the comparison
} else {
connection_->write_and_flush(RequestCallback::Ptr(
new StartupCallback(this, Request::ConstPtr(new QueryRequest("USE " + keyspace_)))));
new StartupCallback(this, Request::ConstPtr(new QueryRequest("USE \"" + keyspace_ + "\"")))));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will break using case intensive keyspaces from the C API.

There are two cases:

  1. User provided keyspaces from the C API these take the form caseinsensitive or \"CaseSensitive\"
  2. Cassandra provided keyspaces (from its metadata) which are will be sent as having the correct case, but i think won't be wrap in the double quotes when case senstivie. I need to verify this.

So it's possible that we should use the form "USE " + keyspace, but properly wrap Cassandra provided keyspaces in double quotes when needed?

Copy link
Contributor

@mpenick mpenick Feb 10, 2020

Choose a reason for hiding this comment

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

The other possibility is that we just need to use "USE \"" + keyspace + "\"" in the right spots e.g. PrepareHostHandler::SetKeyspaceCallback, but not the other two spots as long as we can prove that its keyspace values never come from the C API. We'd also have to prove that the other two spot don't take values from Cassandra provided keyspaces.

Copy link
Author

@derek9gag derek9gag Feb 11, 2020

Choose a reason for hiding this comment

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

I have tried those using cqlsh, assuming we have a "CaseSensitive" and a case_insensitive keyspaces:

  • USE "CaseSensitive"; works
  • USE CaseSensitive; fails
  • USE "case_insensitive"; works
  • USE case_insensitive; works
  • USE CASE_INSENSITIVE; works
  • USE "CASE_INSENSITIVE"; fails

Cassandra provided keyspaces such as system and system_auth are the same as an user case_insensitive keyspace.

Always quoting the namespace only fails when we specify the wrong casing (e.g. USE "System";).

When we retrieve the metadata, the keyspaces will be have the correct casing like system and system_auth, and adding quotes around will work.

Copy link
Author

@derek9gag derek9gag Feb 11, 2020

Choose a reason for hiding this comment

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

I see there is one case-sensitive Cassandra-provided table (system."IndexInfo"), but I have not seen and am not aware of any case-sensitive Cassandra-provided keyspaces.

Copy link
Author

@derek9gag derek9gag Feb 11, 2020

Choose a reason for hiding this comment

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

I tried listing the keyspace names from CassSession schema. The keyspace names are not quoted. I did this via the PHP wrapper:

Listing keyspace names in $session->schema()->keyspaces():
 UpperCase
 lowercase
 system
 system_auth
 system_distributed
 system_schema
 system_traces

I understand that we can do it the other way round (i.e. no quoting in USE <keyspace>;), but I would say that is not the correct approach. First it is counter-intuitive to use, as the quote characters are not part of the keyspace name. Secondly, if you have a program that loops through each keyspace in schema, it will fail on the CaseSensitive ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

The keyspace can't be double quoted in connector.cpp (https://github.com/datastax/cpp-driver/pull/466/files#diff-0fc019678d7d3eecf2cb341a50df9945R265) because it breaks a few cases when you connect with a keyspace.

That's what I was attempting to explain above.

  { // Works
    CassError rc;
    CassSession* session = cass_session_new();
    CassFuture* future = cass_session_connect_keyspace(session, cluster,  "case_insensitive");
    rc = cass_future_error_code(future);
    assert(rc == CASS_OK && "Same case for insensitive should work");
    cass_future_free(future);
    cass_session_free(session);
  }

  { // Should work, but doesn't work: Received error response 'Keyspace 'Case_Insensitive' does not exist' (0x02002200)
    CassError rc;
    CassSession* session = cass_session_new();
    CassFuture* future = cass_session_connect_keyspace(session, cluster,  "Case_Insensitive");
    rc = cass_future_error_code(future);
    assert(rc == CASS_OK && "Different case for insensitive should work");
    cass_future_free(future);
    cass_session_free(session);
  }

  { // Fails (because it's double, double quoted): 'line 1:4 no viable alternative at input '""' (USE [""]...)' (0x02002000)
    CassError rc;
    CassSession* session = cass_session_new();
    CassFuture* future = cass_session_connect_keyspace(session, cluster, "\"CaseSensitive\"");
    rc = cass_future_error_code(future);
    assert(rc == CASS_OK && "Same case with quotes for sensitive should work");
    cass_future_free(future);
    cass_session_free(session);
  }

  { // Works, but shouldn't (it should require double quotes to work)
    CassError rc;
    CassSession* session = cass_session_new();
    CassFuture* future = cass_session_connect_keyspace(session, cluster, "CaseSensitive");
    rc = cass_future_error_code(future);
    assert(rc != CASS_OK && "Same case without quotes for sensitive should not work");
    cass_future_free(future);
    cass_session_free(session);
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a potential fix on a branch here: 72ef57d

Copy link
Author

@derek9gag derek9gag Feb 12, 2020

Choose a reason for hiding this comment

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

I see. I checked out your branch and it works fine for my use-case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a follow up PR (w/ additional tests) #467.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you!

@derek9gag
Copy link
Author

See #467

@derek9gag derek9gag closed this Feb 15, 2020
@derek9gag derek9gag deleted the patch-1 branch March 23, 2020 15:36
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.

2 participants