You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "lowka (via GitHub)" <gi...@apache.org> on 2024/05/07 13:40:16 UTC
[PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
lowka opened a new pull request, #3714:
URL: https://github.com/apache/ignite-3/pull/3714
https://issues.apache.org/jira/browse/IGNITE-21836
---
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1598465731
##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientKeyValueView.java:
##########
@@ -94,18 +97,24 @@ public class ClientKeyValueView<K, V> extends AbstractClientView<Entry<K, V>> im
/** {@inheritDoc} */
@Override
public V get(@Nullable Transaction tx, K key) {
- return sync(getAsync(tx, key));
+ Objects.requireNonNull(key, "key");
+
+ return sync(doGet(tx, key, "getNullable"));
}
/** {@inheritDoc} */
@Override
public CompletableFuture<V> getAsync(@Nullable Transaction tx, K key) {
- Objects.requireNonNull(key);
+ Objects.requireNonNull(key, "key");
+
+ return doGet(tx, key, "getNullableAsync");
Review Comment:
there are a few such places
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1596527346
##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientKeyValueView.java:
##########
@@ -229,6 +235,11 @@ public CompletableFuture<Void> putAllAsync(@Nullable Transaction tx, Map<K, V> p
return nullCompletedFuture();
}
+ for (Entry<K, V> e : pairs.entrySet()) {
+ Objects.requireNonNull(e.getKey());
Review Comment:
should we add some message to help a user identify a problem?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich merged PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1596670082
##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientKeyValueView.java:
##########
@@ -229,6 +235,11 @@ public CompletableFuture<Void> putAllAsync(@Nullable Transaction tx, Map<K, V> p
return nullCompletedFuture();
}
+ for (Entry<K, V> e : pairs.entrySet()) {
+ Objects.requireNonNull(e.getKey());
Review Comment:
We should. But all assertions in this classes and their siblings do not return any message.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1596537490
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/ValidationUtils.java:
##########
@@ -95,5 +99,35 @@ private static boolean isColumnCompatible(BinaryMode colType, Class<?> cls) {
}
}
+ /**
+ * Checks whether {@code null} is allowed for the given value type in a {@code KeyValueView} operation.
+ * If the value type is not natively supported, throws an exception.
+ *
+ * @param val Value to check.
+ * @param valueType Value type.
+ * @throws NullPointerException If value is null and valueType is not natively supported.
+ */
+ public static void validateNullableValue(@Nullable Object val, Class<?> valueType) {
+ if (val == null && !Mapper.nativelySupported(valueType)) {
+ String message = "null value cannot be used when a value is not mapped to a simple type";
+
+ throw new NullPointerException(message);
+ }
+ }
+
+ /**
+ * Checks whether {@code getNullable*} operation on a {@code KeyValueView} is allowed for the given value type.
+ * If the value type is not natively supported, throws an exception.
+ *
+ * @param valueType Value type.
+ */
+ public static void validateNullableOperation(Class<?> valueType) {
+ if (!Mapper.nativelySupported(valueType)) {
+ String message = format("`getNullable`* methods cannot be used when a value is not mapped to a simple type");
Review Comment:
the method is utility and isn't linked to concrete classes that will be used. Maybe better to pass a method name as a parameter to form the message
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1598265024
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientNullsEmbeddedTest.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.runner.app.client;
+
+import org.apache.ignite.Ignite;
+
+/**
+ * Embedded client nulls in operations test.
+ */
+public class ItThinClientNullsEmbeddedTest extends ItThinClientNullsTest {
Review Comment:
client is either _thin_ or _embedded_
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1596683345
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/ValidationUtils.java:
##########
@@ -95,5 +99,35 @@ private static boolean isColumnCompatible(BinaryMode colType, Class<?> cls) {
}
}
+ /**
+ * Checks whether {@code null} is allowed for the given value type in a {@code KeyValueView} operation.
+ * If the value type is not natively supported, throws an exception.
+ *
+ * @param val Value to check.
+ * @param valueType Value type.
+ * @throws NullPointerException If value is null and valueType is not natively supported.
+ */
+ public static void validateNullableValue(@Nullable Object val, Class<?> valueType) {
+ if (val == null && !Mapper.nativelySupported(valueType)) {
+ String message = "null value cannot be used when a value is not mapped to a simple type";
+
+ throw new NullPointerException(message);
+ }
+ }
+
+ /**
+ * Checks whether {@code getNullable*} operation on a {@code KeyValueView} is allowed for the given value type.
+ * If the value type is not natively supported, throws an exception.
+ *
+ * @param valueType Value type.
+ */
+ public static void validateNullableOperation(Class<?> valueType) {
+ if (!Mapper.nativelySupported(valueType)) {
+ String message = format("`getNullable`* methods cannot be used when a value is not mapped to a simple type");
Review Comment:
This code uses already established pattern see `ClientKeyValueView::throwIfNull` its callers.
> throw new UnexpectedNullValueException("Got unexpected null value: use `getNullable` sibling method instead.");
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1596547166
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientNullsTest.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.runner.app.client;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.lang.NullableValue;
+import org.apache.ignite.sql.IgniteSql;
+import org.apache.ignite.table.KeyValueView;
+import org.apache.ignite.table.Table;
+import org.apache.ignite.table.mapper.Mapper;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
+
+/**
+ * Thin client nulls in operations test.
+ */
+public class ItThinClientNullsTest extends ItAbstractThinClientTest {
+
+ static class BoolStr {
+ @SuppressWarnings("unused")
+ boolean boolCol;
+ @SuppressWarnings("unused")
+ String strCol;
+ }
+
+ protected Ignite ignite() {
+ return client();
+ }
+
+ /** Setup. */
+ @BeforeAll
+ public void setup() {
+ IgniteSql sql = ignite().sql();
+
+ sql.execute(null, "CREATE TABLE t0 (ID INTEGER PRIMARY KEY, boolCol BOOLEAN, strCol VARCHAR)").close();
+ sql.execute(null, "INSERT INTO t0 VALUES (1, NULL, NULL)").close();
+
+ sql.execute(null, "CREATE TABLE t1 (ID INTEGER PRIMARY KEY, boolCol BOOLEAN)").close();
+ sql.execute(null, "INSERT INTO t1 VALUES (1, NULL)").close();
+ }
+
+ @Test
+ public void testNullableOpPojo() {
+ Table table = ignite().tables().table("T0");
+ KeyValueView<Integer, BoolStr> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(BoolStr.class));
+
+ expectNotSupported(() -> view.getNullable(null, 1));
+ expectNotSupported(() -> view.getNullableAndPut(null, 1, null));
+ expectNotSupported(() -> view.getNullableAndRemove(null, 1));
+ expectNotSupported(() -> view.getNullableAndReplaceAsync(null, 1, null));
+ }
+
+ @Test
+ public void testModifyOpPoJoNullVal() {
+ Table table = ignite().tables().table("T0");
+ KeyValueView<Integer, BoolStr> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(BoolStr.class));
+
+ expectNullNotAllowed(() -> view.put(null, 2, null));
+ expectNullNotAllowed(() -> view.putIfAbsent(null, 2, null));
+ expectNullNotAllowed(() -> view.replace(null, 2, null));
+ expectNullNotAllowed(() -> view.replace(null, 2, new BoolStr(), null));
+ expectNullNotAllowed(() -> view.getAndPut(null, 2, null));
+ expectNullNotAllowed(() -> view.getAndReplace(null, 2, null));
+ expectNullNotAllowed(() -> {
+ Map<Integer, BoolStr> map = new HashMap<>();
+ map.put(2, null);
+ view.putAll(null, map);
+ });
+ }
+
+ @Test
+ public void testNullableOpSingleCol() {
+ Table table = ignite().tables().table("T1");
+ KeyValueView<Integer, Boolean> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(Boolean.class));
+
+ assertEquals(NullableValue.of(null), view.getNullable(null, 1));
+ assertEquals(NullableValue.of(null), view.getNullableAndPut(null, 1, null));
+ assertEquals(NullableValue.of(null), view.getNullableAndRemove(null, 1));
+ assertNull(view.getNullableAndReplace(null, 1, null));
+ }
+
+ @Test
+ public void testModifyOpSingleColNullVal() {
+ Table table = ignite().tables().table("T1");
+ KeyValueView<Integer, Boolean> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(Boolean.class));
+
+ assertTrue(view.replace(null, 1, null, false));
+ assertEquals(false, view.get(null, 1));
+
+ assertTrue(view.remove(null, 1, false));
+
+ assertTrue(view.putIfAbsent(null, 1, null));
+ view.put(null, 1, true);
+
+ assertEquals(true, view.getAndPut(null, 1, null));
+ assertNull(view.getAndReplace(null, 2, null));
+
+ Map<Integer, Boolean> map = new HashMap<>();
+ map.put(3, null);
+ view.putAll(null, map);
+ assertEquals(NullableValue.of(null), view.getNullable(null, 3));
+ }
+
+ private static void expectNotSupported(Executable exec) {
+ UnsupportedOperationException err = assertThrows(UnsupportedOperationException.class, exec);
+ assertEquals("`getNullable`* methods cannot be used when a value is not mapped to a simple type", err.getMessage());
+ }
+
+ private static void expectNullNotAllowed(Executable exec) {
+ NullPointerException err = assertThrows(NullPointerException.class, exec);
Review Comment:
we have `IgniteTestUtils.assert*` methods
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1596549218
##########
modules/table/src/main/java/org/apache/ignite/internal/table/KeyValueViewImpl.java:
##########
@@ -236,7 +242,11 @@ public void putAll(@Nullable Transaction tx, Map<K, V> pairs) {
public CompletableFuture<Void> putAllAsync(@Nullable Transaction tx, Map<K, V> pairs) {
Objects.requireNonNull(pairs);
for (Entry<K, V> entry : pairs.entrySet()) {
- Objects.requireNonNull(entry.getKey());
+ K key = entry.getKey();
+ V val = entry.getValue();
+
+ Objects.requireNonNull(key);
Review Comment:
https://github.com/apache/ignite-3/pull/3714/files#r1596527346
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1598400961
##########
modules/table/src/main/java/org/apache/ignite/internal/table/KeyValueViewImpl.java:
##########
@@ -236,7 +242,11 @@ public void putAll(@Nullable Transaction tx, Map<K, V> pairs) {
public CompletableFuture<Void> putAllAsync(@Nullable Transaction tx, Map<K, V> pairs) {
Objects.requireNonNull(pairs);
for (Entry<K, V> entry : pairs.entrySet()) {
- Objects.requireNonNull(entry.getKey());
+ K key = entry.getKey();
+ V val = entry.getValue();
+
+ Objects.requireNonNull(key);
Review Comment:
Added variable names to `requireNonNull` calls.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientNullsEmbeddedTest.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.runner.app.client;
+
+import org.apache.ignite.Ignite;
+
+/**
+ * Embedded client nulls in operations test.
+ */
+public class ItThinClientNullsEmbeddedTest extends ItThinClientNullsTest {
Review Comment:
Thanks. Fixed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1599553720
##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientKeyValueView.java:
##########
@@ -94,18 +97,24 @@ public class ClientKeyValueView<K, V> extends AbstractClientView<Entry<K, V>> im
/** {@inheritDoc} */
@Override
public V get(@Nullable Transaction tx, K key) {
- return sync(getAsync(tx, key));
+ Objects.requireNonNull(key, "key");
+
+ return sync(doGet(tx, key, "getNullable"));
}
/** {@inheritDoc} */
@Override
public CompletableFuture<V> getAsync(@Nullable Transaction tx, K key) {
- Objects.requireNonNull(key);
+ Objects.requireNonNull(key, "key");
+
+ return doGet(tx, key, "getNullableAsync");
Review Comment:
No, it should not. It reports what method should be used instead.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1598399757
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientNullsTest.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.runner.app.client;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.lang.NullableValue;
+import org.apache.ignite.sql.IgniteSql;
+import org.apache.ignite.table.KeyValueView;
+import org.apache.ignite.table.Table;
+import org.apache.ignite.table.mapper.Mapper;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
+
+/**
+ * Thin client nulls in operations test.
+ */
+public class ItThinClientNullsTest extends ItAbstractThinClientTest {
+
+ static class BoolStr {
+ @SuppressWarnings("unused")
+ boolean boolCol;
+ @SuppressWarnings("unused")
+ String strCol;
+ }
+
+ protected Ignite ignite() {
+ return client();
+ }
+
+ /** Setup. */
+ @BeforeAll
+ public void setup() {
+ IgniteSql sql = ignite().sql();
+
+ sql.execute(null, "CREATE TABLE t0 (ID INTEGER PRIMARY KEY, boolCol BOOLEAN, strCol VARCHAR)").close();
+ sql.execute(null, "INSERT INTO t0 VALUES (1, NULL, NULL)").close();
+
+ sql.execute(null, "CREATE TABLE t1 (ID INTEGER PRIMARY KEY, boolCol BOOLEAN)").close();
+ sql.execute(null, "INSERT INTO t1 VALUES (1, NULL)").close();
+ }
+
+ @Test
+ public void testNullableOpPojo() {
+ Table table = ignite().tables().table("T0");
+ KeyValueView<Integer, BoolStr> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(BoolStr.class));
+
+ expectNotSupported(() -> view.getNullable(null, 1));
+ expectNotSupported(() -> view.getNullableAndPut(null, 1, null));
+ expectNotSupported(() -> view.getNullableAndRemove(null, 1));
+ expectNotSupported(() -> view.getNullableAndReplaceAsync(null, 1, null));
+ }
+
+ @Test
+ public void testModifyOpPoJoNullVal() {
+ Table table = ignite().tables().table("T0");
+ KeyValueView<Integer, BoolStr> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(BoolStr.class));
+
+ expectNullNotAllowed(() -> view.put(null, 2, null));
+ expectNullNotAllowed(() -> view.putIfAbsent(null, 2, null));
+ expectNullNotAllowed(() -> view.replace(null, 2, null));
+ expectNullNotAllowed(() -> view.replace(null, 2, new BoolStr(), null));
+ expectNullNotAllowed(() -> view.getAndPut(null, 2, null));
+ expectNullNotAllowed(() -> view.getAndReplace(null, 2, null));
+ expectNullNotAllowed(() -> {
+ Map<Integer, BoolStr> map = new HashMap<>();
+ map.put(2, null);
+ view.putAll(null, map);
+ });
+ }
+
+ @Test
+ public void testNullableOpSingleCol() {
+ Table table = ignite().tables().table("T1");
+ KeyValueView<Integer, Boolean> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(Boolean.class));
+
+ assertEquals(NullableValue.of(null), view.getNullable(null, 1));
+ assertEquals(NullableValue.of(null), view.getNullableAndPut(null, 1, null));
+ assertEquals(NullableValue.of(null), view.getNullableAndRemove(null, 1));
+ assertNull(view.getNullableAndReplace(null, 1, null));
+ }
+
+ @Test
+ public void testModifyOpSingleColNullVal() {
+ Table table = ignite().tables().table("T1");
+ KeyValueView<Integer, Boolean> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(Boolean.class));
+
+ assertTrue(view.replace(null, 1, null, false));
+ assertEquals(false, view.get(null, 1));
+
+ assertTrue(view.remove(null, 1, false));
+
+ assertTrue(view.putIfAbsent(null, 1, null));
+ view.put(null, 1, true);
+
+ assertEquals(true, view.getAndPut(null, 1, null));
+ assertNull(view.getAndReplace(null, 2, null));
+
+ Map<Integer, Boolean> map = new HashMap<>();
+ map.put(3, null);
+ view.putAll(null, map);
+ assertEquals(NullableValue.of(null), view.getNullable(null, 3));
+ }
+
+ private static void expectNotSupported(Executable exec) {
+ UnsupportedOperationException err = assertThrows(UnsupportedOperationException.class, exec);
+ assertEquals("`getNullable`* methods cannot be used when a value is not mapped to a simple type", err.getMessage());
+ }
+
+ private static void expectNullNotAllowed(Executable exec) {
+ NullPointerException err = assertThrows(NullPointerException.class, exec);
Review Comment:
fixed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1598464931
##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientKeyValueView.java:
##########
@@ -94,18 +97,24 @@ public class ClientKeyValueView<K, V> extends AbstractClientView<Entry<K, V>> im
/** {@inheritDoc} */
@Override
public V get(@Nullable Transaction tx, K key) {
- return sync(getAsync(tx, key));
+ Objects.requireNonNull(key, "key");
+
+ return sync(doGet(tx, key, "getNullable"));
}
/** {@inheritDoc} */
@Override
public CompletableFuture<V> getAsync(@Nullable Transaction tx, K key) {
- Objects.requireNonNull(key);
+ Objects.requireNonNull(key, "key");
+
+ return doGet(tx, key, "getNullableAsync");
Review Comment:
should it be a
```suggestion
return doGet(tx, key, "getAsync");
```
?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #3714:
URL: https://github.com/apache/ignite-3/pull/3714#discussion_r1596541677
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientNullsTest.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.runner.app.client;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.lang.NullableValue;
+import org.apache.ignite.sql.IgniteSql;
+import org.apache.ignite.table.KeyValueView;
+import org.apache.ignite.table.Table;
+import org.apache.ignite.table.mapper.Mapper;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
+
+/**
+ * Thin client nulls in operations test.
+ */
+public class ItThinClientNullsTest extends ItAbstractThinClientTest {
+
+ static class BoolStr {
+ @SuppressWarnings("unused")
+ boolean boolCol;
+ @SuppressWarnings("unused")
+ String strCol;
+ }
+
+ protected Ignite ignite() {
+ return client();
+ }
+
+ /** Setup. */
+ @BeforeAll
+ public void setup() {
+ IgniteSql sql = ignite().sql();
+
+ sql.execute(null, "CREATE TABLE t0 (ID INTEGER PRIMARY KEY, boolCol BOOLEAN, strCol VARCHAR)").close();
+ sql.execute(null, "INSERT INTO t0 VALUES (1, NULL, NULL)").close();
+
+ sql.execute(null, "CREATE TABLE t1 (ID INTEGER PRIMARY KEY, boolCol BOOLEAN)").close();
+ sql.execute(null, "INSERT INTO t1 VALUES (1, NULL)").close();
+ }
+
+ @Test
+ public void testNullableOpPojo() {
+ Table table = ignite().tables().table("T0");
+ KeyValueView<Integer, BoolStr> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(BoolStr.class));
+
+ expectNotSupported(() -> view.getNullable(null, 1));
+ expectNotSupported(() -> view.getNullableAndPut(null, 1, null));
+ expectNotSupported(() -> view.getNullableAndRemove(null, 1));
+ expectNotSupported(() -> view.getNullableAndReplaceAsync(null, 1, null));
+ }
+
+ @Test
+ public void testModifyOpPoJoNullVal() {
+ Table table = ignite().tables().table("T0");
+ KeyValueView<Integer, BoolStr> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(BoolStr.class));
+
+ expectNullNotAllowed(() -> view.put(null, 2, null));
+ expectNullNotAllowed(() -> view.putIfAbsent(null, 2, null));
+ expectNullNotAllowed(() -> view.replace(null, 2, null));
+ expectNullNotAllowed(() -> view.replace(null, 2, new BoolStr(), null));
+ expectNullNotAllowed(() -> view.getAndPut(null, 2, null));
+ expectNullNotAllowed(() -> view.getAndReplace(null, 2, null));
+ expectNullNotAllowed(() -> {
+ Map<Integer, BoolStr> map = new HashMap<>();
+ map.put(2, null);
+ view.putAll(null, map);
+ });
+ }
+
+ @Test
+ public void testNullableOpSingleCol() {
+ Table table = ignite().tables().table("T1");
+ KeyValueView<Integer, Boolean> view = table.keyValueView(Mapper.of(Integer.class), Mapper.of(Boolean.class));
+
+ assertEquals(NullableValue.of(null), view.getNullable(null, 1));
Review Comment:
maybe we can make NullableValue.NULL is public
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org