spring-data-cassandra icon indicating copy to clipboard operation
spring-data-cassandra copied to clipboard

DATACASS-751: Allow keyspace customization for CassandraPersistentEntity

Open tomekl007 opened this issue 4 years ago • 7 comments

  • [x] You have read the Spring Data contribution guidelines.
  • [x] There is a ticket in the bug tracker for the project in our JIRA.
  • [x] You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • [x] You submit test cases (unit or integration tests) that back your changes.
  • [x] 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 avatar Aug 13 '20 09:08 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

tomekl007 avatar Aug 13 '20 09:08 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?

tomekl007 avatar Aug 14 '20 09:08 tomekl007

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

mp911de avatar Aug 14 '20 09:08 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)?

tomekl007 avatar Aug 27 '20 07:08 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?

tomekl007 avatar Aug 27 '20 08:08 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)?

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 avatar Sep 01 '20 07:09 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.

mp911de avatar Sep 01 '20 07:09 mp911de