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.

Sign up for GitHub

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

Jump to bottom

DATACASS-188 - now able to supply options to table via cassandra admin otemplate #1385

Closed
wants to merge 2 commits into from
Closed

DATACASS-188 - now able to supply options to table via cassandra admin otemplate #1385

wants to merge 2 commits into from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented May 26, 2023

Implements #359
@mp911de, can you please check?)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 26, 2023
mp911de
mp911de requested changes May 30, 2023
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

There are quite some unrelated changes within a single commit. Please split at least the bugfix from the newly introduced method. The polishing changes should be part of a separate commit, can be either PR.

...assandra/src/main/java/org/springframework/data/cassandra/core/CassandraAdminOperations.java Outdated
* @param ifNotExists If true, will only create the table if it doesn't exist, else the create operation will be
* ignored.
* @param tableName The name of the table.
* @param entityClass The class whose fields determine the columns created.
* @param optionsByName Table options, given by the string option name and the appropriate option value.
*/
@Deprecated(forRemoval = true)
Copy link
Member

Choose a reason for hiding this comment

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

We do not intent deprecating this method as a table can be created from the given Class using a different table name.

...-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraAdminTemplate.java
.getCreateTableSpecificationFor(entity, entity.getTableName())
.ifNotExists(ifNotExists);

if (!CollectionUtils.isEmpty(optionsByName)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a bugfix. Can you please split this pull request into two PR's, one for introducing an overload without accepting a table name and the other one to consider options?

...-data-cassandra/src/main/java/org/springframework/data/cassandra/core/CassandraTemplate.java
@@ -961,7 +961,7 @@ protected <E extends CassandraMappingEvent<T>, T> void maybeEmitEvent(Supplier<E
protected <T> T maybeCallBeforeConvert(T object, CqlIdentifier tableName) {

if (null != entityCallbacks) {
return (T) entityCallbacks.callback(BeforeConvertCallback.class, object, tableName);
return entityCallbacks.callback(BeforeConvertCallback.class, object, tableName);
Copy link
Member

Choose a reason for hiding this comment

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

These should go into a separate commit as well to separate polishing from the actual change.

@mp911de mp911de self-assigned this May 30, 2023
@mipo256
Copy link
Contributor Author

mipo256 commented May 30, 2023

@mp911de So I will just break this PR into 3 different PRs:

  1. Actual bugfix
  2. Creation of new overloaded method
  3. Polishing/refactoring

Am I correct?

@mp911de
Copy link
Member

mp911de commented May 30, 2023

1 + 2 are sufficient. Feel free to have another commit with polishing in one of the two pull requests. If you like, you can reuse this PR for the bugfix and create a new one for the newly introduced method. This change would require rebase and force-pushing which is fine for now.

@mipo256
Copy link
Contributor Author

mipo256 commented May 30, 2023

@mp911de I have removed new method from this PR. Also I have introduced 2 commits - one with polishing, another with actual bugfix. Can you please re-review?

P.S: I will introduce new method once this changes will occur in the main branch

@mipo256 mipo256 requested a review from mp911de May 30, 2023 15:56
@mipo256
Copy link
Contributor Author

mipo256 commented Jun 5, 2023

Mark @mp911de, when you will be able to rereview this PR?) I have already created a separate brunch for new overloaded method, but I am waiting for these changes to occur in main brunch to create new PR as we discussed

@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 7, 2023
@mp911de mp911de added this to the 3.4.13 (2021.2.13) milestone Jun 7, 2023
mp911de pushed a commit that referenced this pull request Jun 7, 2023
@mipo256 @mp911de
Correctly apply TableOptions through CassandraAdminTemplate.createTab…
f6e56e4 
…le(…).

Closes: #359
Original pull request: #1385
mp911de pushed a commit that referenced this pull request Jun 7, 2023
@mipo256 @mp911de
Polishing.
d1d2840 
See: #359
Original pull request: #1385
mp911de added a commit that referenced this pull request Jun 7, 2023
@mp911de
Polishing.
ca17fde 
Add Javadoc. Reformat code.

See: #359
Original pull request: #1385
mp911de pushed a commit that referenced this pull request Jun 7, 2023
@mipo256 @mp911de
Correctly apply TableOptions through CassandraAdminTemplate.createTab…
6c193c6 
…le(…).

Closes: #359
Original pull request: #1385
mp911de pushed a commit that referenced this pull request Jun 7, 2023
@mipo256 @mp911de
Polishing.
cf47ae4 
See: #359
Original pull request: #1385
mp911de added a commit that referenced this pull request Jun 7, 2023
@mp911de
Polishing.
b1f91f6 
Add Javadoc. Reformat code.

See: #359
Original pull request: #1385
mp911de pushed a commit that referenced this pull request Jun 7, 2023
@mipo256 @mp911de
Correctly apply TableOptions through CassandraAdminTemplate.createTab…
cd444b2 
…le(…).

Closes: #359
Original pull request: #1385
mp911de pushed a commit that referenced this pull request Jun 7, 2023
@mipo256 @mp911de
Polishing.
1d3a485 
See: #359
Original pull request: #1385
mp911de added a commit that referenced this pull request Jun 7, 2023
@mp911de
Polishing.
2d250bb 
Add Javadoc. Reformat code.

See: #359
Original pull request: #1385
mp911de pushed a commit that referenced this pull request Jun 7, 2023
@mipo256 @mp911de
Correctly apply TableOptions through CassandraAdminTemplate.createTab…
aa07755 
…le(…).

Closes: #359
Original pull request: #1385
mp911de added a commit that referenced this pull request Jun 7, 2023
@mp911de
Polishing.
0c77ac6 
Add Javadoc. Reformat code.

See: #359
Original pull request: #1385
@mp911de
Copy link
Member

mp911de commented Jun 7, 2023

Thank you for your contribution. That's merged, polished, and backported now.

@mp911de mp911de closed this Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de Awaiting requested review from mp911de

Assignees

@mp911de mp911de

Labels
type: bug A general bug
Projects
None yet
Milestone
3.4.13 (2021.2.13)
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mipo256 @mp911de @spring-projects-issues