DATACASS-751: Allow keyspace customization for CassandraPersistentEntity

[tomekl007]

• You have read the Spring Data contribution guidelines.
• There is a ticket in the bug tracker for the project in our JIRA.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.
• You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

todo:

• adapt Events (i.e BeforeDeleteEvent, BeforeSaveEvent, AfterSaveEvent, etc) to TableCoordinates
• cover TableCoordinates with keyspace with more IT and UnitTests
• consider CassandraTemplate#getMapper to work per TableCoordinates (not only tableName)
• consider moving TableCoordinates to CassandraPersistentEntity#getTableCoordinates() AND BasicCassandraPersistentEntity

[tomekl007]

I am opening this as a draft PR to discuss if the changes are in the right direction.

Right now I modified only QueryBuilder.selectFrom to make it work for keyspace and table. The changes are backward compatible, and the keyspace can be null.
The keyspace is propagated from the NamingStrategy:
https://github.com/spring-projects/spring-data-cassandra/pull/179/files#diff-5ffacbd4e7f86a4f70fc9d41510316a7R58
and is tied to theCassandraPersistentEntity as discussed in the JIRA ticket (https://github.com/spring-projects/spring-data-cassandra/pull/179/files#diff-f553f0af64a53e7dd83c965ec8362159R231)
@mp911de let me know wdyt if this is going in the right direction.
If yes, I will proceed in the same way with:

• QueryBuilder.truncate
• QueryBuilder.insertInto
• QueryBuilder.update
• QueryBuilder.deleteFrom

methods

[mp911de]

This PR goes into a good direction. Introducing the keyspace as additional element surfaces the fact that we suddenly deal with two components to address a particular table (getKeyspaceName(), getTableName()).

Having both pieces of information can lead to a disjoint when one of them isn't matching the other one (e.g. ks1.mytable, where ks1 gets resolved to a different value and then mytable can't be found anymore).

The natural fix is to introduce a value object which encapsulates both values so that they are bundled together. However, we should also consider the getTableName() method which currently returns CqlIdentifier instead of e.g. TableName. Such a change would have two consequences:

1. It would break existing API
2. It creates another problem, that setTableName(…) accepts CqlIdentifier while getTableName(…) would return TableName.

I think we can solve this issue by coming up with a better name.

[tomekl007]

One more question @mp911de.
The logic for do*Versioned is emitting events like for example:
maybeEmitEvent(new AfterSaveEvent<>(entityToSave, tableCoordinates.getTableName()));
and:

public AfterSaveEvent(E source, CqlIdentifier tableName) {
     super(source, tableName);
}

It seems that we would need to modify events to send TableCoordinates instead of only tableName, right?

[mp911de]

Let's keep events and entity callbacks for a future step in mind so we don't change these yet.

[tomekl007]

re: getMapper() method and it's invocations.
The mapper currently is created per Table as we discussed. It creates one problem.
For example, looking at a place where TableCoordinates are used:

<T> List<T> doSelect(Query query, Class<?> entityClass, TableCoordinates tableCoordinates,
			Class<T> returnType) {

and the actual logic of getting mapper is happening per table:

	Function<Row, T> mapper = getMapper(entityClass, returnType, tableCoordinates.getTableName());

So it basically means that TableCoordinates.keyspaceName is ignored for all CassandraTemplate methods that are using the mapper.
It means that the behavior of methods that are using mapper vs those that are using statements directly (created by the StatementFactory) is different.

It may lead to in-deterministic behavior if the client has a table with the same name in two different keyspaces. The client will be able to execute per ks queries using Statements, but when it comes to a mapper, only one table could be retrieved and the keyspace is inherited from the CqlSession. Are we ok with this behavior? Or should we create mapper per keyspace-table as well?

[tomekl007]

I think that we should consider adding the getTableCoordinates() at the level of CassandraPersistentEntity.
The reason for this is that the TableCooordinates are used now in a lot of places in code, and the callers of this class need to wrap tableName and keyspaceName into TableCoordinates. This wrapping (constructing TableCooordinates) is happening in a lot of places and having this method one level higher will remove the need to construct it a lot of times, wdyt?

[tomekl007]

Let's keep events and entity callbacks for a future step in mind so we don't change these yet.

What do you mean by the future step? Do you want to do it in a separate ticket (PR)?

[tomekl007]

question regarding testing:
I would like to add tests for all *OperationSupport classes that are working now per TableCoordinates, and also have a new method:
UpdateWithQuery inTable(CqlIdentifier keyspaceName, CqlIdentifier tableName)
So for example adding new tests to ExecutableUpdateOperationSupportIntegrationTests.
To test the new per kespace.table behavior, I would like to use CassandraAdminTemplate to create a table in the specific keyspace. Right now, the CassandraAdminTemplate is exposing methods that are working only for table and keyspace cannot be specified explicitly, i.e: 'public void dropTable(CqlIdentifier tableName), public void createTable(boolean ifNotExists, CqlIdentifier tableName.... I think that we should consider adding new methods to CassandraAdminTemplatethat are working perkeyspace/table`. They will be used to write good integration tests, but also end-users may find it useful. wdyt?

[mp911de]

Let's keep events and entity callbacks for a future step in mind so we don't change these yet.

What do you mean by the future step? Do you want to do it in a separate ticket (PR)?

Yes, I'd like to not introduce another level of complexity with this pull request but rather to postpone changes to events and entity callbacks to after this PR is merged.

[mp911de]

It makes sense to extend CassandraAdminTemplate by introducing overloads accepting TableCooordinates. Going further, I'm wondering about UDT's since they have a name and they exist in a keyspace. Is it possible to do something similar as with tables, so that a UDT gets created and referenced in a different keyspace?

I'm also wondering whether our TableCoordinates-approach is the right thing to do. In Spring Data JDBC, we faced a similar request, to prefix tables with a schema name and we've built SqlIdentifier that can be either a simple (table/column) or a composite identifier (schema with table).

In the Cassandra case, we're referencing to the Driver CqlIdentifier. I'm a bit hesitant to subclass CqlIdentifer for representing table name and keyspace name as the type isn't under our control and the Driver could decide to make this type final. Additionally, it's a class and not an interface which makes it a bit more complex to create subclasses.

The gist here is, if CqlIdentifier would be a bit more friendly to extend, then we could simplify this PR a lot more. Paging @adutra for further discussion.