FMWK-261 Align find APIs to use Query instead of Qualifier #652

agrgr · 2023-11-05T13:43:54Z

findUsingQuery() instead of findByQualifier(), deprecate AerospikeCriteria, Qualifier implements CriteriaDefinition

…ate AerospikeCriteria, Qualifier implements CriteriaDefinition

# Conflicts: # src/main/java/org/springframework/data/aerospike/query/KeyQualifier.java # src/main/java/org/springframework/data/aerospike/repository/support/SimpleAerospikeRepository.java # src/test/java/org/springframework/data/aerospike/repository/PersonRepositoryQueryTests.java

roimenashe · 2023-11-06T15:27:16Z

@agrgr What about Operations/Template (for both reactive and non-reactive) changes? we need to replace all findUsingQualifier methods with findUsingQuery alternatives and still need to decide what to do with existing find() variations that accepts Query (should add new method and mark as deprecated or just remove).

We shouldn't have any find..Qualifier (ById, Using, findAll...) exposed in our interfaces/implementations after migrating to Query.

…e flow

agrgr · 2023-11-06T16:20:56Z

We shouldn't have any find..Qualifier (ById, Using, findAll...) exposed in our interfaces/implementations after migrating to Query.

Right, there was findByQualifier() in reactive flow which is now updated.

roimenashe · 2023-11-06T16:25:04Z

We shouldn't have any find..Qualifier (ById, Using, findAll...) exposed in our interfaces/implementations after migrating to Query.

Right, there was findByQualifier() in reactive flow which is now updated.

We still have bunch of Qualifier usages in AerospikeOperations/AerospikeTemplate and also in reactive AerospikeReactiveOperations/AerospikeReactiveTemplate, I think we should get rid of Qualifier completely at these levels (repository and operations/template) we should use Query in API everywhere and translate it into qualifiers only internally (private methods of the implementations [templates]).

Let's talk about it tomorrow.

agrgr · 2023-11-06T16:34:17Z

I think we should get rid of Qualifier completely at these levels (repository and operations/template) we should use Query in API everywhere

Ok, firstly I thought this PR would handle just replacing findByQualifier. Went back to your description, makes sense. Will add an update aligning methods.

reugn · 2023-11-08T07:26:56Z

src/main/java/org/springframework/data/aerospike/core/AerospikeOperations.java

     * @return Stream of entities.
     */
-    <T, S> Stream<?> findUsingQualifier(Class<T> entityClass, Class<S> targetClass, @Nullable Filter filter,
-                                        Qualifier qualifier);
+    <T, S> Stream<?> find(Query query, Class<T> entityClass, Class<S> targetClass, @Nullable Filter filter);


return Stream<S>

reugn · 2023-11-08T07:29:11Z

src/main/java/org/springframework/data/aerospike/core/ReactiveAerospikeOperations.java

+     * @param filter      Secondary index filter.
+     * @return Stream of entities.
+     */
+    <T, S> Flux<?> find(Query query, Class<T> entityClass, Class<S> targetClass, @Nullable Filter filter);


return Flux<S>

reugn · 2023-11-08T07:34:04Z

src/main/java/org/springframework/data/aerospike/query/QualifierUtils.java

+        if (criteria == null) {
+            return null;
+        } else if (criteria.getQualifiers() == null) {
+            return new Qualifier[]{(criteria)};


(criteria) -> criteria

roimenashe · 2023-11-08T08:24:57Z

src/main/java/org/springframework/data/aerospike/core/AerospikeOperations.java

@@ -712,11 +712,11 @@ public interface AerospikeOperations {
     * @param id          The id of the document to find. Must not be {@literal null}.
     * @param entityClass The class to extract the Aerospike set from. Must not be {@literal null}.
     * @param targetClass The class to map the document to.
-     * @param qualifiers  {@link Qualifier}s provided to build a filter Expression for the query. Optional argument.
+     * @param query       {@link Query} provided to build a filter expression. Optional argument.


...to build a filter expression is not accurate, filters could also lead to statement filters and Query might also contain sorting and limiting options (same for other query param descriptions in javadocs)

roimenashe · 2023-11-08T08:26:02Z

src/main/java/org/springframework/data/aerospike/core/AerospikeOperations.java

     * @return Stream of entities.
     */
-    <T> Stream<T> findUsingQualifier(Class<T> entityClass, @Nullable Filter filter, Qualifier qualifier);
+    <T> Stream<T> find(Query query, Class<T> entityClass, @Nullable Filter filter);


Do we still need the filter? filtering (both statement and expressions) comes from the Qualifier within the Query, consult with @reugn wether we should still expose Filter as a param.

Yes, having the Query parameter only should be enough.

Done. For future: there in an opportunity to add a condition in StatementBuilder to check Filter first and not build Qualifier-constructed Filter if it already exists

Note that this might be a breaking change if we had Filter exposed in Template/Operations/Repository, talked with @agrgr and agreed to verify that, if its a breaking change it might be the first one for this release (which may require major version release).

roimenashe · 2023-11-08T09:37:51Z

src/main/java/org/springframework/data/aerospike/core/AerospikeTemplate.java

        try {
            AerospikePersistentEntity<?> entity = mappingContext.getRequiredPersistentEntity(entityClass);
            Key key = getKey(id, setName);

            if (targetClass != null && targetClass != entityClass) {
-                return getRecordMapToTargetClass(entity, key, targetClass, qualifiers);
+                return getRecordMapToTargetClass(entity, key, targetClass, criteria);


Only used in one place and we are passing criteria now so no need for this method to accept Qualifier... (varargs) but a single Qualifier object, try to see how deep it goes, wherever you can - use Qualifier instead of Qualifier... (varargs).

roimenashe · 2023-11-08T09:42:23Z

src/main/java/org/springframework/data/aerospike/repository/query/AerospikePartTreeQuery.java

+            List<Object> ids;
+            if (criteria.hasSingleId()) {
+                ids = getIdValue(criteria);
+                return operations.findByIdsUsingQuery(ids, entityClass, targetClass, (Query) null);


Redundant (Query) casting

roimenashe · 2023-11-08T09:43:33Z

...ain/java/org/springframework/data/aerospike/repository/query/BaseAerospikePartTreeQuery.java

@@ -62,8 +60,8 @@ protected Query prepareQuery(Object[] parameters, ParametersParameterAccessor ac
        PartTree tree = new PartTree(queryMethod.getName(), entityClass);
        Query baseQuery = createQuery(accessor, tree);

-        AerospikeCriteria criteria = baseQuery.getAerospikeCriteria();
-        Query query = new Query(criteria);
+        Qualifier qualifiers = baseQuery.getCriteria().getCriteriaObject();


qualifier instead of qualifiers

roimenashe · 2023-11-08T09:46:49Z

src/test/java/org/springframework/data/aerospike/sample/PersonRepository.java

+     * Find all entities that have primary key in the given list.
+     *
+     * @param ids List of primary keys
+     */
    List<P> findById(List<String> ids);


Why findById and not findByIds if it gets a list of ids? same for many methods in this class

Because it is in repository, so it needs the exact name of the field

…nd QueryEngine

add support for findUsingQuery() instead of findByQualifier(), deprec…

577ae88

…ate AerospikeCriteria, Qualifier implements CriteriaDefinition

agrgr added the enhancement label Nov 5, 2023

agrgr requested review from roimenashe and reugn November 5, 2023 13:43

agrgr added 3 commits November 6, 2023 15:15

deprecate Query(Sort sort) constructor

5d44a0e

merge updates from the main branch

c487c1c

agrgr marked this pull request as ready for review November 6, 2023 13:34

agrgr added 2 commits November 6, 2023 15:36

polish code format

823e746

polish wording

1ec57f1

add support for findUsingQuery instead of findByQualifier for reactiv…

479b1bb

…e flow

align public "find" APIs to use Query instead of Qualifiers

dd1e1bd

reugn reviewed Nov 8, 2023

View reviewed changes

roimenashe reviewed Nov 8, 2023

View reviewed changes

agrgr added 6 commits November 8, 2023 11:49

cleanup

9a283aa

remove public find methods that receive Filter parameter, cleanup

0f91c05

cleanup

5e219fd

add clarification for nullable query parameter in find methods

2dce86e

cleanup

4bed2e0

use correct annotation

9dbba3a

reugn approved these changes Nov 8, 2023

View reviewed changes

switch to receiving Query instead of Qualifiers in StatementBuilder a…

a2983c7

…nd QueryEngine

agrgr changed the title ~~FMWK-261 Add support for findUsingQuery instead of findByQualifier~~ FMWK-261 Align find APIs to use Query instead of Qualifier Nov 8, 2023

agrgr added 6 commits November 9, 2023 08:09

fix tests

44d7137

remove Filter parameter from QueryEngine and StatementBuilder

00d0bc9

cleanup

8b3f47e

remove EXCLUDE_FILTER in Qualifier

2127cb5

fix for a test

66003eb

cleanup

8d46025

roimenashe approved these changes Nov 9, 2023

View reviewed changes

agrgr merged commit 579a204 into main Nov 9, 2023
5 checks passed

agrgr deleted the FMWK-261-findUsingQuery branch November 9, 2023 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FMWK-261 Align find APIs to use Query instead of Qualifier #652

FMWK-261 Align find APIs to use Query instead of Qualifier #652

agrgr commented Nov 5, 2023

roimenashe commented Nov 6, 2023

agrgr commented Nov 6, 2023

roimenashe commented Nov 6, 2023 •

edited

Loading

agrgr commented Nov 6, 2023 •

edited

Loading

reugn Nov 8, 2023

reugn Nov 8, 2023

reugn Nov 8, 2023

roimenashe Nov 8, 2023

roimenashe Nov 8, 2023

reugn Nov 8, 2023

agrgr Nov 8, 2023

roimenashe Nov 8, 2023

roimenashe Nov 8, 2023

roimenashe Nov 8, 2023

roimenashe Nov 8, 2023

roimenashe Nov 8, 2023

agrgr Nov 8, 2023

FMWK-261 Align find APIs to use Query instead of Qualifier #652

FMWK-261 Align find APIs to use Query instead of Qualifier #652

Conversation

agrgr commented Nov 5, 2023

roimenashe commented Nov 6, 2023

agrgr commented Nov 6, 2023

roimenashe commented Nov 6, 2023 • edited Loading

agrgr commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roimenashe commented Nov 6, 2023 •

edited

Loading

agrgr commented Nov 6, 2023 •

edited

Loading