Skip to content

Commit 3995472

Browse files
bsboddenbsbodden
committed
fix: complete NUMERIC_IN/NOT_IN implementation with proper query generation
- Fix NUMERIC_IN to not escape numeric values (dots in decimals were being escaped) - Add missing NUMERIC_NOT_IN implementation using RediSearch negation pattern - Add support for arrays/varargs parameters in collection queries - Handle empty collections correctly (NUMERIC_IN returns no results, NOT_IN returns all) - Fix query combination for multiple values using proper OR syntax with parentheses test: add comprehensive tests for numeric IN/NOT_IN queries - Add NumericInQueryTest with 17 test cases covering all numeric types - Add NumericIndexedIdFieldTest to verify @NumericIndexed on @id fields - Add NumericInQueryClauseTest for enum verification - Add metamodel generator test for @NumericIndexed ID fields - Test edge cases: empty collections, single values, large numbers, nullables Fixes issues found in PR #645 where NUMERIC_IN had syntax errors and NUMERIC_NOT_IN was not implemented. All tests now pass.
1 parent bcb7461 commit 3995472

File tree

10 files changed

+680
-6
lines changed

10 files changed

+680
-6
lines changed

‎redis-om-spring/src/main/java/com/redis/om/spring/repository/query/clause/QueryClause.java

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -577,8 +577,26 @@ public String prepareQuery(String field, Object... params) {
577577
prepared = prepared.replace(PARAM_PREFIX + i++, ObjectUtils.getDistanceAsRedisString(distance));
578578
break;
579579
default:
580-
// unfold collections
581-
if (param instanceof Collection<?> c) {
580+
// unfold collections and arrays
581+
Collection<?> c = null;
582+
if (param instanceof Collection<?>) {
583+
c = (Collection<?>) param;
584+
} else if (param != null && param.getClass().isArray()) {
585+
// Convert array to collection
586+
if (param instanceof Object[]) {
587+
c = Arrays.asList((Object[]) param);
588+
} else {
589+
// Handle primitive arrays
590+
int length = java.lang.reflect.Array.getLength(param);
591+
List<Object> list = new ArrayList<>(length);
592+
for (int j = 0; j < length; j++) {
593+
list.add(java.lang.reflect.Array.get(param, j));
594+
}
595+
c = list;
596+
}
597+
}
598+
599+
if (c != null) {
582600
String value;
583601
if (this == QueryClause.TAG_CONTAINING_ALL) {
584602
value = c.stream().map(n -> "@" + field + ":{" + QueryUtils.escape(ObjectUtils.asString(n,
@@ -590,10 +608,35 @@ public String prepareQuery(String field, Object... params) {
590608
.joining("|"));
591609
prepared = prepared.replace(PARAM_PREFIX + i++, value);
592610
} else if (this == QueryClause.NUMERIC_IN) {
593-
value = c.stream().map(n -> "@" + field + ":[" + QueryUtils.escape(ObjectUtils.asString(n,
594-
converter)) + " " + QueryUtils.escape(ObjectUtils.asString(n, converter)) + "]").collect(Collectors
595-
.joining("|"));
596-
prepared = value;
611+
// For numeric values, don't escape - use toString() directly
612+
if (c.isEmpty()) {
613+
// Empty collection means no matches - return impossible query
614+
prepared = prepared.replace(PARAM_PREFIX + i++, "[-1 -2]"); // Impossible range
615+
} else if (c.size() == 1) {
616+
// Single value - simple range query
617+
Object val = c.iterator().next();
618+
prepared = prepared.replace(PARAM_PREFIX + i++, "[" + val.toString() + " " + val.toString() + "]");
619+
} else {
620+
// Multiple values - need OR query with parentheses
621+
value = "(" + c.stream().map(n -> "@" + field + ":[" + n.toString() + " " + n.toString() + "]").collect(
622+
Collectors.joining("|")) + ")";
623+
// Replace the entire prepared string with the OR query
624+
prepared = value;
625+
}
626+
} else if (this == QueryClause.NUMERIC_NOT_IN) {
627+
// For NUMERIC_NOT_IN, we need to exclude the values
628+
// RediSearch doesn't have a direct NOT IN, so we use a negation pattern
629+
if (c.isEmpty()) {
630+
// Empty collection means match everything with this field
631+
// Use the full numeric range query
632+
prepared = "@" + field + ":[-inf inf]";
633+
} else {
634+
// Create a query that excludes specific values
635+
// Replace the entire query with a combination of field existence and exclusions
636+
value = "@" + field + ":[-inf inf] " + c.stream().map(n -> "-@" + field + ":[" + n.toString() + " " + n
637+
.toString() + "]").collect(Collectors.joining(" "));
638+
prepared = value;
639+
}
597640
} else if (this == QueryClause.NUMERIC_CONTAINING_ALL) {
598641
value = c.stream().map(n -> "@" + field + ":[" + QueryUtils.escape(ObjectUtils.asString(n,
599642
converter)) + " " + QueryUtils.escape(ObjectUtils.asString(n, converter)) + "]").collect(Collectors

‎tests/src/test/java/com/redis/om/spring/fixtures/document/model/NumericIdTestEntity.java

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package com.redis.om.spring.fixtures.document.model;
2+
3+
import org.springframework.data.annotation.Id;
4+
5+
import com.redis.om.spring.annotations.Document;
6+
import com.redis.om.spring.annotations.NumericIndexed;
7+
import com.redis.om.spring.annotations.TagIndexed;
8+
9+
import lombok.AccessLevel;
10+
import lombok.AllArgsConstructor;
11+
import lombok.Data;
12+
import lombok.NoArgsConstructor;
13+
import lombok.RequiredArgsConstructor;
14+
import lombok.NonNull;
15+
16+
@Data
17+
@NoArgsConstructor
18+
@RequiredArgsConstructor
19+
@AllArgsConstructor(access = AccessLevel.PROTECTED)
20+
@Document
21+
public class NumericIdTestEntity {
22+
@Id
23+
@NumericIndexed // Testing @NumericIndexed on @Id field
24+
private Long id;
25+
26+
@NonNull
27+
@TagIndexed
28+
private String name;
29+
30+
@NonNull
31+
@NumericIndexed
32+
private Integer value;
33+
}

‎tests/src/test/java/com/redis/om/spring/fixtures/document/model/NumericInTestEntity.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package com.redis.om.spring.fixtures.document.model;
2+
3+
import org.springframework.data.annotation.Id;
4+
5+
import com.redis.om.spring.annotations.Document;
6+
import com.redis.om.spring.annotations.NumericIndexed;
7+
import com.redis.om.spring.annotations.TagIndexed;
8+
9+
import lombok.AccessLevel;
10+
import lombok.AllArgsConstructor;
11+
import lombok.Data;
12+
import lombok.NoArgsConstructor;
13+
import lombok.RequiredArgsConstructor;
14+
import lombok.NonNull;
15+
16+
@Data
17+
@NoArgsConstructor
18+
@RequiredArgsConstructor
19+
@AllArgsConstructor(access = AccessLevel.PROTECTED)
20+
@Document
21+
public class NumericInTestEntity {
22+
@Id
23+
private String id;
24+
25+
@NonNull
26+
@TagIndexed
27+
private String name;
28+
29+
@NonNull
30+
@NumericIndexed
31+
private Integer age;
32+
33+
@NonNull
34+
@NumericIndexed
35+
private Long score;
36+
37+
@NonNull
38+
@NumericIndexed
39+
private Double rating;
40+
41+
@NumericIndexed
42+
private Integer level;
43+
}

‎tests/src/test/java/com/redis/om/spring/fixtures/document/repository/NumericIdTestEntityRepository.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package com.redis.om.spring.fixtures.document.repository;
2+
3+
import java.util.Collection;
4+
import java.util.List;
5+
6+
import com.redis.om.spring.fixtures.document.model.NumericIdTestEntity;
7+
import com.redis.om.spring.repository.RedisDocumentRepository;
8+
9+
public interface NumericIdTestEntityRepository extends RedisDocumentRepository<NumericIdTestEntity, Long> {
10+
// Test querying by numeric ID
11+
List<NumericIdTestEntity> findByIdIn(Collection<Long> ids);
12+
List<NumericIdTestEntity> findByIdNotIn(Collection<Long> ids);
13+
14+
// Combined queries with numeric ID
15+
List<NumericIdTestEntity> findByIdInAndName(Collection<Long> ids, String name);
16+
List<NumericIdTestEntity> findByValueIn(Collection<Integer> values);
17+
}

‎tests/src/test/java/com/redis/om/spring/fixtures/document/repository/NumericInTestEntityRepository.java

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package com.redis.om.spring.fixtures.document.repository;
2+
3+
import java.util.Collection;
4+
import java.util.List;
5+
import java.util.Set;
6+
7+
import com.redis.om.spring.fixtures.document.model.NumericInTestEntity;
8+
import com.redis.om.spring.repository.RedisDocumentRepository;
9+
10+
public interface NumericInTestEntityRepository extends RedisDocumentRepository<NumericInTestEntity, String> {
11+
// Test methods for NUMERIC_IN
12+
List<NumericInTestEntity> findByAgeIn(Collection<Integer> ages);
13+
List<NumericInTestEntity> findByAgeIn(Set<Integer> ages);
14+
List<NumericInTestEntity> findByAgeIn(List<Integer> ages);
15+
List<NumericInTestEntity> findByAgeIn(Integer... ages);
16+
17+
// Test methods for Long type
18+
List<NumericInTestEntity> findByScoreIn(Collection<Long> scores);
19+
List<NumericInTestEntity> findByScoreIn(Long... scores);
20+
21+
// Test methods for Double type
22+
List<NumericInTestEntity> findByRatingIn(Collection<Double> ratings);
23+
List<NumericInTestEntity> findByRatingIn(Double... ratings);
24+
25+
// Test methods for NUMERIC_NOT_IN
26+
List<NumericInTestEntity> findByAgeNotIn(Collection<Integer> ages);
27+
List<NumericInTestEntity> findByScoreNotIn(Collection<Long> scores);
28+
List<NumericInTestEntity> findByRatingNotIn(Collection<Double> ratings);
29+
30+
// Combined queries
31+
List<NumericInTestEntity> findByAgeInAndScoreIn(Collection<Integer> ages, Collection<Long> scores);
32+
List<NumericInTestEntity> findByNameAndAgeIn(String name, Collection<Integer> ages);
33+
34+
// Edge cases
35+
List<NumericInTestEntity> findByLevelIn(Collection<Integer> levels); // nullable field
36+
}

‎tests/src/test/java/com/redis/om/spring/indexing/NumericIndexedIdFieldTest.java

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
package com.redis.om.spring.indexing;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.junit.jupiter.api.Assertions.*;
5+
6+
import java.util.Arrays;
7+
import java.util.List;
8+
9+
import org.junit.jupiter.api.AfterEach;
10+
import org.junit.jupiter.api.BeforeEach;
11+
import org.junit.jupiter.api.Test;
12+
import org.springframework.beans.factory.annotation.Autowired;
13+
14+
import com.redis.om.spring.AbstractBaseDocumentTest;
15+
import com.redis.om.spring.fixtures.document.model.NumericIdTestEntity;
16+
import com.redis.om.spring.fixtures.document.repository.NumericIdTestEntityRepository;
17+
18+
class NumericIndexedIdFieldTest extends AbstractBaseDocumentTest {
19+
20+
@Autowired
21+
NumericIdTestEntityRepository repository;
22+
23+
private NumericIdTestEntity entity1;
24+
private NumericIdTestEntity entity2;
25+
private NumericIdTestEntity entity3;
26+
27+
@BeforeEach
28+
void setUp() {
29+
// Create test entities with numeric IDs
30+
entity1 = new NumericIdTestEntity("Entity One", 100);
31+
entity1.setId(1001L);
32+
33+
entity2 = new NumericIdTestEntity("Entity Two", 200);
34+
entity2.setId(1002L);
35+
36+
entity3 = new NumericIdTestEntity("Entity Three", 100);
37+
entity3.setId(1003L);
38+
39+
repository.saveAll(Arrays.asList(entity1, entity2, entity3));
40+
}
41+
42+
@AfterEach
43+
void tearDown() {
44+
repository.deleteAll();
45+
}
46+
47+
@Test
48+
void testNumericIndexedIdFieldDoesNotCauseDuplicateSchema() {
49+
// This test verifies that having @NumericIndexed on an @Id field
50+
// doesn't cause duplicate schema field errors
51+
52+
// If the fix in RediSearchIndexer is working correctly,
53+
// the entities should be saved and queryable without issues
54+
55+
// Verify entities were saved
56+
assertThat(repository.count()).isEqualTo(3);
57+
58+
// Verify we can query by ID
59+
assertThat(repository.findById(1001L)).isPresent();
60+
assertThat(repository.findById(1002L)).isPresent();
61+
assertThat(repository.findById(1003L)).isPresent();
62+
}
63+
64+
@Test
65+
void testFindByIdIn() {
66+
List<Long> ids = Arrays.asList(1001L, 1003L);
67+
List<NumericIdTestEntity> results = repository.findByIdIn(ids);
68+
69+
assertThat(results).hasSize(2);
70+
assertThat(results).contains(entity1, entity3);
71+
assertThat(results).doesNotContain(entity2);
72+
}
73+
74+
@Test
75+
void testFindByIdNotIn() {
76+
List<Long> ids = Arrays.asList(1001L, 1002L);
77+
List<NumericIdTestEntity> results = repository.findByIdNotIn(ids);
78+
79+
assertThat(results).hasSize(1);
80+
assertThat(results).contains(entity3);
81+
assertThat(results).doesNotContain(entity1, entity2);
82+
}
83+
84+
@Test
85+
void testFindByIdInAndName() {
86+
List<Long> ids = Arrays.asList(1001L, 1002L, 1003L);
87+
List<NumericIdTestEntity> results = repository.findByIdInAndName(ids, "Entity Two");
88+
89+
assertThat(results).hasSize(1);
90+
assertThat(results).contains(entity2);
91+
}
92+
93+
@Test
94+
void testFindByValueIn() {
95+
List<Integer> values = Arrays.asList(100, 300);
96+
List<NumericIdTestEntity> results = repository.findByValueIn(values);
97+
98+
// entity1 and entity3 have value=100
99+
assertThat(results).hasSize(2);
100+
assertThat(results).contains(entity1, entity3);
101+
assertThat(results).doesNotContain(entity2);
102+
}
103+
104+
@Test
105+
void testNumericIdWithLargeValues() {
106+
// Test with larger ID values
107+
NumericIdTestEntity largeIdEntity = new NumericIdTestEntity("Large ID Entity", 500);
108+
largeIdEntity.setId(999999999L);
109+
repository.save(largeIdEntity);
110+
111+
List<Long> ids = Arrays.asList(999999999L, 1001L);
112+
List<NumericIdTestEntity> results = repository.findByIdIn(ids);
113+
114+
assertThat(results).hasSize(2);
115+
assertThat(results).contains(largeIdEntity, entity1);
116+
117+
repository.delete(largeIdEntity);
118+
}
119+
120+
@Test
121+
void testNumericIdQueryPerformance() {
122+
// Create more entities for performance testing
123+
for (long i = 2000L; i < 2100L; i++) {
124+
NumericIdTestEntity entity = new NumericIdTestEntity("Entity " + i, (int)(i % 10));
125+
entity.setId(i);
126+
repository.save(entity);
127+
}
128+
129+
// Query with a large set of IDs
130+
List<Long> ids = Arrays.asList(1001L, 1002L, 1003L, 2001L, 2050L, 2099L);
131+
List<NumericIdTestEntity> results = repository.findByIdIn(ids);
132+
133+
assertThat(results).hasSize(6);
134+
135+
// Clean up additional entities
136+
for (long i = 2000L; i < 2100L; i++) {
137+
repository.deleteById(i);
138+
}
139+
}
140+
}

‎tests/src/test/java/com/redis/om/spring/metamodel/MetamodelGeneratorTest.java

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -490,6 +490,40 @@ void testValidDocumentNumericIndexedComplex(Results results) throws IOException
490490
);
491491
}
492492

493+
@Test
494+
@Classpath(
495+
"data.metamodel.ValidDocumentNumericIndexedId"
496+
)
497+
void testValidDocumentNumericIndexedId(Results results) throws IOException {
498+
List<String> warnings = getWarningStrings(results);
499+
assertThat(warnings).isEmpty();
500+
501+
List<String> errors = getErrorStrings(results);
502+
assertThat(errors).isEmpty();
503+
504+
assertThat(results.generated).hasSize(1);
505+
JavaFileObject metamodel = results.generated.get(0);
506+
assertThat(metamodel.getName()).isEqualTo("/SOURCE_OUTPUT/valid/ValidDocumentNumericIndexedId$.java");
507+
508+
var fileContents = metamodel.getCharContent(true);
509+
510+
assertAll( //
511+
// Test that @NumericIndexed on @Id field generates proper metamodel
512+
// Note: ID field with @NumericIndexed is generated as TextTagField (this is the current behavior)
513+
() -> assertThat(fileContents).contains("public static TextTagField<ValidDocumentNumericIndexedId, Long> ID;"),
514+
() -> assertThat(fileContents).contains("public static TextTagField<ValidDocumentNumericIndexedId, String> NAME;"),
515+
() -> assertThat(fileContents).contains("public static NumericField<ValidDocumentNumericIndexedId, Integer> VALUE;"),
516+
517+
// Test proper field initialization with SearchFieldAccessor
518+
() -> assertThat(fileContents).contains(
519+
"ID = new TextTagField<ValidDocumentNumericIndexedId, Long>(new SearchFieldAccessor(\"id\", \"$.id\", id),true);"),
520+
() -> assertThat(fileContents).contains(
521+
"NAME = new TextTagField<ValidDocumentNumericIndexedId, String>(new SearchFieldAccessor(\"name\", \"$.name\", name),true);"),
522+
() -> assertThat(fileContents).contains(
523+
"VALUE = new NumericField<ValidDocumentNumericIndexedId, Integer>(new SearchFieldAccessor(\"value\", \"$.value\", value),true);")
524+
);
525+
}
526+
493527
@Test
494528
@Classpath(
495529
"data.metamodel.ValidDocumentNumericIndexed"

0 commit comments

Comments
 (0)