JAVA-2307: Improve @Select and @Delete by not requiring full primary key #1285

tolbertam · 2019-06-27T18:08:15Z

Motivation:

Currently @Select and @Delete annotated methods only allow
selecting/deleting individual entities when parameters are provided as
the mapper currently requires specifying all columns of the primary key as
parameters.

Since CQL allows you to select and delete multiple rows by providing a
subset of the primary key, these dao methods can be improved by
accepting a subset of the primary key as parameters, as long as
at least the partition keys are represented.

In addition @Select should be supported when providing no parameters,
which will allow querying an entire table's rows for a given entity.

Modifications:

Add EntityHelper.selectByPrimaryKeyParts and deleteByPrimaryKeyParts
methods and added generators to provide this capability.

Refactor parameter validation into EntityUtils.areParametersValid from
somewhat duplicated logic in delete and select method generators.

Add tests to exercise functionality.

Add documentation demonstrating extended capability.

Result:

@Select and @Delete now support providing a subset of primary key to
select/delete multiple entities.

@Select also supports providing zero parameters, which performs a
select on all rows in an entity's table.

Motivation: Currently `@Select` and `@Delete` annotated methods only allow selecting/deleting individual entities when parameters are provided as the mapper currently requires specifying all columns of the primary key as parameters. Since CQL allows you to select and delete multiple rows by providing a subset of the primary key, these dao methods can be improved by accepting a subset of the primary key as parameters, as long as at least the partition keys are represented. In addition `@Select` should be supported when providing no parameters, which will allow querying an entire table's rows for a given entity. Modifications: Add `EntityHelper.selectByPrimaryKeyParts` and `deleteByPrimaryKeyParts` methods and added generators to provide this capability. Refactor parameter validation into EntityUtils.areParametersValid from somewhat duplicated logic in delete and select method generators. Add tests to exercise functionality. Add documentation demonstrating extended capability. Result: `@Select` and `@Delete` now support providing a subset of primary key to select/delete multiple entities. `@Select` also supports providing zero parameters, which performs a select on all rows in an entity's table.

tolbertam · 2019-06-27T18:11:40Z

mapper-runtime/src/main/java/com/datastax/oss/driver/api/mapper/entity/EntityHelper.java

+   */
+  @NonNull
+  default Select selectByPrimaryKeyParts(int parameterCount) {
+    throw new UnsupportedOperationException("selectbyPrimaryKeyParts is unimplemented");


Done to avoid breaking changes, although it is a bit awkward.

Done to avoid breaking changes, although it is a bit awkward.

Yes, and this only hides the breaking change by turning it into a runtime error. I've been thinking about other possible approaches:

introduce a sub-interface and require a cast (as we've done in other cases). The problem is that this doesn't scale well: we add ExtendedProductHelper now but what if we need more methods in the next version? ExtendedExtendedProductHelper?

Accept the breaking change and document it, on the assumption that clients probably don't implement this interface. The problem is in the "probably"; also, it's a slippery slope.

Don't add those methods to EntityHelper. Instead, generate the query directly from the method generators. For SELECT we already have selectStart(); for DELETE we have to duplicate the whole method, but it's not huge either.

We won't make those queries available to users through the interface (e.g. for custom query providers), but they're not that hard to rewrite from scratch anyway.

My vote goes to 3.

Don't add those methods to EntityHelper

You know, it didn't occur to me that generated code used the implementations instead of declaring things as EntityHelper. In this case, there is no reason to add something to the interface, so I agree this seems like a good course of action.

It looks like the only place (currently) that EntityHelper is used as an interface in other code is in QueryProvider and in DaoBase, where it's more used for setting/getting fields.

Since this method is used for query generation, I suppose it's probably fine to not put directly in EntityHelper.

But I do wonder if we should just consider removing all the query-generation based methods from EntityHelper now if we see no utility. What do you think?

You know, it didn't occur to me that generated code used the implementations instead of declaring things as EntityHelper.

It didn't occur to me either 😆 I was initially thinking of moving the code to the DAO implementation class, but keeping them as "internal" methods in the helper class is even better.

removing all the query-generation based methods from EntityHelper

As we discussed privately, those methods can be useful for @QueryProvider implementors, so it's nice to have them on the interface.

adutra · 2019-06-28T09:14:25Z

changelog/README.md

@@ -2,8 +2,10 @@

 <!-- Note: contrary to 3.x, insert new entries *first* in their section -->

+


Unnecessary change.

adutra · 2019-06-28T09:15:46Z

changelog/README.md

 ### 4.2.0 (in progress)

+- [improvement] JAVA-2307: Improve @Select and @Delete by not requiring full primary key


Tip: place all words starting with @ within backticks here and in commit messages, to avoid having Github consider that these are user handles.

Thanks, I'll amend the commit when merging. It looks like github doesn't hijack markdown in the same way, but agree that we shoulds use backticks here as well.

integration-tests/src/test/java/com/datastax/oss/driver/mapper/InventoryITBase.java

manual/mapper/daos/delete/README.md

...ain/java/com/datastax/oss/driver/internal/mapper/processor/dao/DaoDeleteMethodGenerator.java

...ocessor/src/main/java/com/datastax/oss/driver/internal/mapper/processor/dao/EntityUtils.java

...ain/java/com/datastax/oss/driver/internal/mapper/processor/dao/DaoDeleteMethodGenerator.java

olim7t · 2019-06-30T21:51:41Z

mapper-runtime/src/main/java/com/datastax/oss/driver/api/mapper/entity/EntityHelper.java

+   */
+  @NonNull
+  default Select selectByPrimaryKeyParts(int parameterCount) {
+    throw new UnsupportedOperationException("selectbyPrimaryKeyParts is unimplemented");


Done to avoid breaking changes, although it is a bit awkward.

Yes, and this only hides the breaking change by turning it into a runtime error. I've been thinking about other possible approaches:

introduce a sub-interface and require a cast (as we've done in other cases). The problem is that this doesn't scale well: we add ExtendedProductHelper now but what if we need more methods in the next version? ExtendedExtendedProductHelper?

Accept the breaking change and document it, on the assumption that clients probably don't implement this interface. The problem is in the "probably"; also, it's a slippery slope.

Don't add those methods to EntityHelper. Instead, generate the query directly from the method generators. For SELECT we already have selectStart(); for DELETE we have to duplicate the whole method, but it's not huge either.

We won't make those queries available to users through the interface (e.g. for custom query providers), but they're not that hard to rewrite from scratch anyway.

My vote goes to 3.

…l/mapper/processor/dao/EntityUtils.java Co-Authored-By: Olivier Michallat <olim7t@users.noreply.github.com>

* Use backticks for annotations in changelog. * Use constants for dates in tests. * Make more explicit in readme that clustering column order matters. * Rename keyParameterSize -> primaryKeyParamterCounter * sublist primaryKeyNames to avoid assertion error. * Demote deleteByPrimaryKeyParts / selectByPrimaryKeyParts from EntityHelper interface. *

olim7t

LGTM

olim7t · 2019-07-01T17:35:19Z

manual/mapper/daos/delete/README.md

+    void deleteByIdForCustomer(UUID productId, LocalDate day, UUID customerId);
+
+    /* Note that the clustering columns in your primary key definition are significant. All
+     * proceeding clustering columns must be provided if any are.


proceeding clustering columns

Did you mean "preceding"?

🤦‍♂️ yep, that's what I meant. Good catch, i'll fix.

tolbertam requested a review from olim7t June 27, 2019 18:08

Merge branch '4.x' into java2307

fafe348

tolbertam commented Jun 27, 2019

View reviewed changes

adutra reviewed Jun 28, 2019

View reviewed changes

olim7t suggested changes Jun 30, 2019

View reviewed changes

tolbertam and others added 3 commits July 1, 2019 08:42

Update mapper-processor/src/main/java/com/datastax/oss/driver/interna…

7213c04

…l/mapper/processor/dao/EntityUtils.java Co-Authored-By: Olivier Michallat <olim7t@users.noreply.github.com>

Merge branch '4.x' into java2307

8aa689f

olim7t approved these changes Jul 1, 2019

View reviewed changes

tolbertam added 2 commits July 1, 2019 13:20

proceding -> preceding

f11e87b

Merge branch '4.x' into java2307

99e78cc

tolbertam merged commit f26cae8 into 4.x Jul 1, 2019

tolbertam deleted the java2307 branch July 1, 2019 19:38

tolbertam added this to the 4.2.0 milestone Jul 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JAVA-2307: Improve @Select and @Delete by not requiring full primary key #1285

JAVA-2307: Improve @Select and @Delete by not requiring full primary key #1285

Uh oh!

tolbertam commented Jun 27, 2019

Uh oh!

tolbertam Jun 27, 2019

Uh oh!

olim7t Jun 30, 2019

Uh oh!

tolbertam Jul 1, 2019

Uh oh!

olim7t Jul 1, 2019

Uh oh!

adutra Jun 28, 2019

Uh oh!

adutra Jun 28, 2019

Uh oh!

tolbertam Jul 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olim7t Jun 30, 2019

Uh oh!

olim7t left a comment

Uh oh!

olim7t Jul 1, 2019

Uh oh!

tolbertam Jul 1, 2019

Uh oh!

Uh oh!

		@@ -2,8 +2,10 @@

		<!-- Note: contrary to 3.x, insert new entries first in their section -->

		### 4.2.0 (in progress)

		- [improvement] JAVA-2307: Improve @Select and @Delete by not requiring full primary key

JAVA-2307: Improve @Select and @Delete by not requiring full primary key #1285

JAVA-2307: Improve @Select and @Delete by not requiring full primary key #1285

Uh oh!

Conversation

tolbertam commented Jun 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olim7t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!