Copilot commented on code in PR #13583:
URL: https://github.com/apache/ignite/pull/13583#discussion_r4313035322
##########
modules/core/src/main/java/org/apache/ignite/configuration/CacheConfiguration.java:
##########
@@ -2484,6 +2493,26 @@ public CacheConfiguration<K, V> setIndexPath(String
idxPath) {
return S.toString(CacheConfiguration.class, this);
}
+ /** */
+ void replaceQueryEntities(Collection<QueryEntity> qryEntities) {
+ this.qryEntities = new ArrayList<>(qryEntities);
+ }
+
+ /** */
+ private void
checkQueryEntityConfigurationSource(QueryEntityConfigurationSource src) {
+ if (qryEntityCfgSrc != null && qryEntityCfgSrc != src)
+ throw new
CacheException(String.format(MIXED_QUERY_ENTITIES_API_ERROR_TEMPLATE, name));
Review Comment:
If `name` hasn’t been set yet, the thrown message will contain
`cacheName=null`, which is not very actionable. Consider falling back to a
placeholder like `<unnamed>` or deriving the effective cache name (if available
at that stage) to improve diagnostics.
##########
modules/core/src/main/java/org/apache/ignite/configuration/CacheConfigurationInternalAccessor.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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
+ *
+ * http://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.apache.ignite.configuration;
+
+import java.util.Collection;
+import org.apache.ignite.cache.QueryEntity;
+
+/** Provides internal access to {@link CacheConfiguration} implementation
details. */
+public final class CacheConfigurationInternalAccessor {
+ /** */
+ private CacheConfigurationInternalAccessor() {
+ // No-op.
+ }
+
+ /**
+ * Replaces query entities without changing the query-entity configuration
source.
+ *
+ * @param cfg Cache configuration.
+ * @param qryEntities Query entities.
+ */
+ public static void replaceQueryEntities(CacheConfiguration<?, ?> cfg,
Collection<QueryEntity> qryEntities) {
+ cfg.replaceQueryEntities(qryEntities);
+ }
+}
Review Comment:
This introduces a new `public` type in a public API package
(`org.apache.ignite.configuration`) that is intended for internal use. To avoid
unintentionally expanding the supported public surface, consider marking it
explicitly as internal (project annotation/javadoc conventions), and/or
limiting visibility if the project has a pattern for internal helpers in public
packages.
##########
modules/spring/src/test/java/org/apache/ignite/internal/CacheConfigurationQueryEntityXmlTest.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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
+ *
+ * http://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.apache.ignite.internal;
+
+import javax.cache.CacheException;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.internal.util.typedef.X;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.springframework.beans.PropertyAccessException;
+import org.springframework.beans.PropertyBatchUpdateException;
+
+import static
org.apache.ignite.configuration.CacheConfiguration.MIXED_QUERY_ENTITIES_API_ERROR_TEMPLATE;
+import static org.apache.ignite.testframework.GridTestUtils.assertThrows;
+
+/** Tests that Spring XML configuration cannot mix indexed types and explicit
query entities. */
+public class CacheConfigurationQueryEntityXmlTest extends
GridCommonAbstractTest {
+ /** */
+ private static final String CONFIG_DIR =
"modules/spring/src/test/config/query-entities/";
Review Comment:
Using a hard-coded relative path for test resources can make the test
sensitive to the JVM working directory (e.g., when executed from a different
module or via IDE). Prefer resolving via Ignite home/test resource resolution
utilities (e.g., resolving from classpath or `U.resolveIgnitePath(...)`) so the
test is location-independent.
##########
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCacheAbstractQuerySelfTest.java:
##########
@@ -1186,7 +1219,7 @@ public void testTwoObjectsTextSearch() {
new QueryEntity(String.class, ObjectValueOther.class)
));
- IgniteCache<Object, Object> c = jcache(ignite(), conf, Object.class,
Object.class);
+ IgniteCache<Object, Object> c = jcache(ignite(), conf,
"Object-Object");
Review Comment:
This call relies on a `jcache(...)` overload taking `(Ignite,
CacheConfiguration, String)`. In this diff, a dedicated helper
`jcacheWithSpecifiedName(...)` is introduced and used elsewhere for explicit
naming; using that helper here would make the intent unambiguous and reduce
coupling to overload resolution.
##########
modules/core/src/main/java/org/apache/ignite/configuration/CacheConfiguration.java:
##########
@@ -2075,42 +2090,36 @@ public CacheConfiguration<K, V>
setPartitionLossPolicy(PartitionLossPolicy partL
/**
* Sets query entities configuration.
+ * <p>
+ * This method must not be used together with {@link #setIndexedTypes}.
+ * <p>
+ * Repeated calls replace the query entities configured by the previous
call.
+ * Passing an empty collection clears the existing query entities.
*
* @param qryEntities Query entities.
* @return {@code this} for chaining.
+ * @throws CacheException If query entities have already been configured
through {@link #setIndexedTypes}.
*/
public CacheConfiguration<K, V> setQueryEntities(Collection<QueryEntity>
qryEntities) {
- if (this.qryEntities == null) {
- this.qryEntities = new ArrayList<>(qryEntities);
-
- return this;
- }
+
checkQueryEntityConfigurationSource(QueryEntityConfigurationSource.QUERY_ENTITIES);
- for (QueryEntity entity : qryEntities) {
- boolean found = false;
+ this.qryEntities = new ArrayList<>(qryEntities);
- for (QueryEntity existing : this.qryEntities) {
- if (Objects.equals(entity.findValueType(),
existing.findValueType())) {
- found = true;
-
- break;
- }
- }
-
- if (!found)
- this.qryEntities.add(entity);
- }
+ qryEntityCfgSrc = QueryEntityConfigurationSource.QUERY_ENTITIES;
return this;
}
Review Comment:
`setQueryEntities` will throw a `NullPointerException` if called with `null`
because of `new ArrayList<>(qryEntities)`. Since the method previously didn’t
document nullability (and other setters often treat null/empty as 'clear'),
consider either: (1) explicitly rejecting null with a clear
`IllegalArgumentException`/`CacheException`, or (2) treating null as 'clear'
(equivalent to empty), to avoid an unexpected NPE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
