From 0ba8e45bef4166113ad7103db8c651fa1ac3a742 Mon Sep 17 00:00:00 2001 From: John Blum Date: Tue, 16 Feb 2021 18:05:07 -0800 Subject: [PATCH] Fix bug with SDG Repository derived queries using the IN operator with numeric values. Resolves gh-483. --- .../gemfire/repository/query/QueryString.java | 40 +++-- .../StringBasedGemfireRepositoryQuery.java | 12 +- src/test/java/example/app/model/User.java | 58 +++++++ .../java/example/app/repo/UserRepository.java | 49 ++++++ .../query/QueryStringUnitTests.java | 4 +- ...yQueryUsingInOperatorIntegrationTests.java | 152 ++++++++++++++++++ 6 files changed, 295 insertions(+), 20 deletions(-) create mode 100644 src/test/java/example/app/model/User.java create mode 100644 src/test/java/example/app/repo/UserRepository.java create mode 100644 src/test/java/org/springframework/data/gemfire/repository/query/RepositoryQueryUsingInOperatorIntegrationTests.java diff --git a/src/main/java/org/springframework/data/gemfire/repository/query/QueryString.java b/src/main/java/org/springframework/data/gemfire/repository/query/QueryString.java index 25a1f793..ad80eb1e 100644 --- a/src/main/java/org/springframework/data/gemfire/repository/query/QueryString.java +++ b/src/main/java/org/springframework/data/gemfire/repository/query/QueryString.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2020 the original author or authors. + * Copyright 2012-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,8 +16,6 @@ package org.springframework.data.gemfire.repository.query; -import static org.springframework.data.gemfire.util.CollectionUtils.nullSafeIsEmpty; - import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -28,6 +26,8 @@ import org.apache.geode.cache.Region; import org.springframework.data.domain.Sort; import org.springframework.data.gemfire.repository.query.support.OqlKeyword; +import org.springframework.data.gemfire.util.CollectionUtils; +import org.springframework.lang.NonNull; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; @@ -59,9 +59,10 @@ public class QueryString { private static final String TRACE_OQL_TEMPLATE = " %1$s"; // OQL Query Regular Expression Patterns - private static final String IN_PATTERN = "(?<=IN (SET|LIST) )\\$\\d"; - private static final String IN_PARAMETER_PATTERN = "(?<=IN (SET|LIST) \\$)\\d"; - private static final String REGION_PATTERN = "\\/(\\/?\\w)+"; + protected static final String IN_PATTERN = "(?<=IN (SET|LIST) )\\$\\d"; + protected static final String IN_PARAMETER_PATTERN = "(?<=IN (SET|LIST) \\$)\\d"; + protected static final String IN_VALUES_TEMPLATE = "(%s)"; + protected static final String REGION_PATTERN = "\\/(\\/?\\w)+"; private static final String COUNT_QUERY = "count(*)"; private static final String STAR_QUERY = "*"; @@ -193,17 +194,28 @@ public class QueryString { } /** - * Binds the given {@link Collection} of values into the {@literal IN} parameters of the OQL Query by expanding - * the given values into a comma-separated {@link String}. + * Binds the given {@link Collection} of values into the first {@literal IN} parameter of the OQL Query + * ({@link String}) by expanding the given values into a comma-separated list. * - * @param values the values to bind, returns the {@link QueryString} as is if {@literal null} is given. - * @return a Query String having "in" parameters bound with values. + * @param values {@link Collection} of values to bind; must not be {@literal null} or {@literal empty}. + * @return a new {@link QueryString} having {@literal IN} parameter bound with values + * or returns this {@link QueryString} if the {@link Collection} of values is {@literal null} or {@literal empty}. + * @see java.util.Collection */ - public QueryString bindIn(Collection values) { + public @NonNull QueryString bindIn(@NonNull Collection values) { - if (!nullSafeIsEmpty(values)) { - return QueryString.of(this.query.replaceFirst(IN_PATTERN, String.format("(%s)", - StringUtils.collectionToDelimitedString(values, ", ", "'", "'")))); + if (!CollectionUtils.nullSafeIsEmpty(values)) { + + boolean isNumeric = values.stream().anyMatch(Number.class::isInstance); + + String delimiter = ", "; + String prefix = isNumeric ? "" : "'"; + String suffix = prefix; + + String query = this.query.replaceFirst(IN_PATTERN, String.format(IN_VALUES_TEMPLATE, + StringUtils.collectionToDelimitedString(values, delimiter, prefix, suffix))); + + return QueryString.of(query); } return this; diff --git a/src/main/java/org/springframework/data/gemfire/repository/query/StringBasedGemfireRepositoryQuery.java b/src/main/java/org/springframework/data/gemfire/repository/query/StringBasedGemfireRepositoryQuery.java index 334f8d64..e81661f6 100644 --- a/src/main/java/org/springframework/data/gemfire/repository/query/StringBasedGemfireRepositoryQuery.java +++ b/src/main/java/org/springframework/data/gemfire/repository/query/StringBasedGemfireRepositoryQuery.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2020 the original author or authors. + * Copyright 2012-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,6 +26,7 @@ import org.apache.geode.cache.query.SelectResults; import org.springframework.dao.IncorrectResultSizeDataAccessException; import org.springframework.data.gemfire.GemfireTemplate; import org.springframework.data.repository.Repository; +import org.springframework.data.repository.query.Parameters; import org.springframework.data.repository.query.ParametersParameterAccessor; import org.springframework.data.repository.query.QueryMethod; import org.springframework.data.repository.query.RepositoryQuery; @@ -166,8 +167,10 @@ public class StringBasedGemfireRepositoryQuery extends GemfireRepositoryQuery { query = isUserDefinedQuery() ? query : query.fromRegion(queryMethod.getEntityInformation().getJavaType(), getTemplate().getRegion()); + Parameters queryMethodParameters = queryMethod.getParameters(); + ParametersParameterAccessor parameterAccessor = - new ParametersParameterAccessor(queryMethod.getParameters(), arguments); + new ParametersParameterAccessor(queryMethodParameters, arguments); for (Integer index : query.getInParameterIndexes()) { query = query.bindIn(toCollection(parameterAccessor.getBindableValue(index - 1))); @@ -226,7 +229,7 @@ public class StringBasedGemfireRepositoryQuery extends GemfireRepositoryQuery { Collection toCollection(Object source) { if (source instanceof SelectResults) { - return ((SelectResults) source).asList(); + return ((SelectResults) source).asList(); } if (source instanceof Collection) { @@ -240,6 +243,7 @@ public class StringBasedGemfireRepositoryQuery extends GemfireRepositoryQuery { return source.getClass().isArray() ? CollectionUtils.arrayToList(source) : Collections.singletonList(source); } + @SuppressWarnings("rawtypes") enum ProvidedQueryPostProcessors implements QueryPostProcessor { HINT { @@ -283,7 +287,7 @@ public class StringBasedGemfireRepositoryQuery extends GemfireRepositoryQuery { @Override public String postProcess(QueryMethod queryMethod, String query, Object... arguments) { - if (queryMethod instanceof GemfireQueryMethod) { + if (queryMethod instanceof GemfireQueryMethod) { GemfireQueryMethod gemfireQueryMethod = (GemfireQueryMethod) queryMethod; diff --git a/src/test/java/example/app/model/User.java b/src/test/java/example/app/model/User.java new file mode 100644 index 00000000..2e1a669c --- /dev/null +++ b/src/test/java/example/app/model/User.java @@ -0,0 +1,58 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package example.app.model; + +import org.springframework.data.annotation.Id; +import org.springframework.data.gemfire.mapping.annotation.Region; +import org.springframework.lang.NonNull; +import org.springframework.lang.Nullable; + +import lombok.AccessLevel; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.RequiredArgsConstructor; +import lombok.Setter; +import lombok.ToString; + +/** + * Abstract Data Type (ADT) modeling a user. + * + * @author John Blum + * @see org.springframework.data.gemfire.mapping.annotation.Region + * @since 2.5.0 + */ +@Getter +@ToString +@EqualsAndHashCode +@Setter(AccessLevel.PROTECTED) +@Region("Users") +@NoArgsConstructor +@RequiredArgsConstructor(staticName = "as") +@SuppressWarnings("unused") +public class User { + + @Id + private Integer id; + + @lombok.NonNull + private String name; + + public @NonNull User identifiedBy(@Nullable Integer id) { + setId(id); + return this; + } +} diff --git a/src/test/java/example/app/repo/UserRepository.java b/src/test/java/example/app/repo/UserRepository.java new file mode 100644 index 00000000..70a3f67c --- /dev/null +++ b/src/test/java/example/app/repo/UserRepository.java @@ -0,0 +1,49 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package example.app.repo; + +import java.util.List; +import java.util.Set; + +import org.springframework.data.gemfire.repository.GemfireRepository; + +import example.app.model.User; + +/** + * Spring Data [Geode] {@link GemfireRepository} used to perform basic CRUD and simple OQL query data access operations + * on {@link User} objects to and from an Apache Geode {@link org.apache.geode.cache.GemFireCache} + * {@link org.apache.geode.cache.Region}. + * + * @author John Blum + * @see org.apache.geode.cache.GemFireCache + * @see org.apache.geode.cache.Region + * @see org.springframework.data.gemfire.repository.GemfireRepository + * @see example.app.model.User + * @since 2.5.0 + */ +@SuppressWarnings("unused") +public interface UserRepository extends GemfireRepository { + + List findByIdInOrderById(Integer... identifiers); + + //@Query("SELECT u FROM /Users u WHERE u.id IN ($1) ORDER BY u.id") + List findByIdInOrderById(Set identifiers); + + List findByNameInOrderByName(String... usersnames); + + List findByNameInOrderByName(Set usernames); + +} diff --git a/src/test/java/org/springframework/data/gemfire/repository/query/QueryStringUnitTests.java b/src/test/java/org/springframework/data/gemfire/repository/query/QueryStringUnitTests.java index 3a7beb1a..c2ff18b0 100644 --- a/src/test/java/org/springframework/data/gemfire/repository/query/QueryStringUnitTests.java +++ b/src/test/java/org/springframework/data/gemfire/repository/query/QueryStringUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2020 the original author or authors. + * Copyright 2012-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -308,7 +308,7 @@ public class QueryStringUnitTests { QueryString query = QueryString.of("SELECT * FROM /Collection WHERE elements IN SET $1"); assertThat(query.bindIn(Arrays.asList(1, 2, 3)).toString()) - .isEqualTo("SELECT * FROM /Collection WHERE elements IN SET ('1', '2', '3')"); + .isEqualTo("SELECT * FROM /Collection WHERE elements IN SET (1, 2, 3)"); } @Test diff --git a/src/test/java/org/springframework/data/gemfire/repository/query/RepositoryQueryUsingInOperatorIntegrationTests.java b/src/test/java/org/springframework/data/gemfire/repository/query/RepositoryQueryUsingInOperatorIntegrationTests.java new file mode 100644 index 00000000..1ffdc4e0 --- /dev/null +++ b/src/test/java/org/springframework/data/gemfire/repository/query/RepositoryQueryUsingInOperatorIntegrationTests.java @@ -0,0 +1,152 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.gemfire.repository.query; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Arrays; +import java.util.Comparator; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.apache.geode.cache.client.ClientRegionShortcut; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.gemfire.config.annotation.ClientCacheApplication; +import org.springframework.data.gemfire.config.annotation.EnableEntityDefinedRegions; +import org.springframework.data.gemfire.repository.config.EnableGemfireRepositories; +import org.springframework.data.gemfire.util.ArrayUtils; +import org.springframework.data.gemfire.util.CollectionUtils; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringRunner; + +import example.app.model.User; +import example.app.repo.UserRepository; + +/** + * Integration Tests testing and asserting the use of the {@literal IN} operator in an OQL query + * + * @author John Blum + * @see org.junit.Test + * @see org.springframework.data.gemfire.repository.GemfireRepository + * @see org.springframework.data.gemfire.repository.config.EnableGemfireRepositories + * @see org.springframework.test.context.ContextConfiguration + * @see org.springframework.test.context.junit4.SpringRunner + * @see Fix bug with SDG Repository derived queries using the IN operator with numeric values. + * @since 2.5.0 + */ +@RunWith(SpringRunner.class) +@ContextConfiguration +@SuppressWarnings("unused") +public class RepositoryQueryUsingInOperatorIntegrationTests { + + private static final AtomicBoolean initialized = new AtomicBoolean(false); + + private List users = Arrays.asList( + User.as("jonDoe").identifiedBy(1), + User.as("janeDoe").identifiedBy(2), + User.as("cookieDoe").identifiedBy(3), + User.as("pieDoe").identifiedBy(4), + User.as("sourDoe").identifiedBy(5) + ); + + @Autowired + private UserRepository userRepository; + + private static boolean doOnce() { + return initialized.compareAndSet(false, true); + } + + @Before + public void setup() { + + assertThat(this.userRepository).isNotNull(); + + if (doOnce()) { + this.userRepository.saveAll(this.users); + } + + assertThat(this.userRepository.count()).isEqualTo(this.users.size()); + } + + private User[] findUsersByIds(Integer... ids) { + + return this.users.stream() + .filter(user -> Arrays.asList(ArrayUtils.nullSafeArray(ids, Integer.class)).contains(user.getId())) + .sorted(Comparator.comparing(User::getId)) + .toArray(User[]::new); + } + + private User[] findUsersByNames(String... names) { + + return this.users.stream() + .filter(user -> Arrays.asList(ArrayUtils.nullSafeArray(names, String.class)).contains(user.getName())) + .sorted(Comparator.comparing(User::getName)) + .toArray(User[]::new); + } + + @Test + public void findByUserIdsInArrayIsCorrect() { + + List usersById = this.userRepository.findByIdInOrderById(2, 4); + + assertThat(usersById).isNotNull(); + assertThat(usersById).hasSize(2); + assertThat(usersById).containsExactly(findUsersByIds(2, 4)); + } + + @Test + public void findByUserIdsInSetIsCorrect() { + + List usersById = this.userRepository.findByIdInOrderById(CollectionUtils.asSet(2, 4, 3)); + + assertThat(usersById).isNotNull(); + assertThat(usersById).hasSize(3); + assertThat(usersById).containsExactlyInAnyOrder(findUsersByIds(2, 3, 4)); + } + + @Test + public void findByUserNamesInArrayIsCorrect() { + + List usersByName = + this.userRepository.findByNameInOrderByName("pieDoe", "cookieDoe", "sourDoe"); + + assertThat(usersByName).isNotNull(); + assertThat(usersByName).hasSize(3); + assertThat(usersByName).containsExactly(findUsersByNames("cookieDoe", "pieDoe", "sourDoe")); + } + + @Test + public void findByUserNamesInSetIsCorrect() { + + List usersByName = + this.userRepository.findByNameInOrderByName(CollectionUtils.asSet("sourDoe", "jonDoe")); + + assertThat(usersByName).isNotNull(); + assertThat(usersByName).hasSize(2); + assertThat(usersByName).containsExactly(findUsersByNames("jonDoe", "sourDoe")); + } + + @ClientCacheApplication(name = "RepositoryQueryUsingInOperatorIntegrationTests") + @EnableEntityDefinedRegions(basePackageClasses = User.class, clientRegionShortcut = ClientRegionShortcut.LOCAL) + @EnableGemfireRepositories(basePackageClasses = UserRepository.class) + static class TestGeodeConfiguration { } + +}