IGNITE-21836: KeyValueView. GetNullable produces confusing error for a PoJo when field / column nullability do not match #3714
Conversation
lowka
force-pushed
the
ignite-21836
branch
7 times, most recently
from
ce785f5
to
9d03cb4
Compare
…a PoJo when field / column nullability do not match
| @@ -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()); | |||
There was a problem hiding this comment.
should we add some message to help a user identify a problem?
There was a problem hiding this comment.
We should. But all assertions in this classes and their siblings do not return any message.
| */ | ||
| 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"); |
There was a problem hiding this 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
There was a problem hiding this comment.
This code uses already established pattern see ClientKeyValueView::throwIfNull its callers.
throw new UnexpectedNullValueException("Got unexpected null value: use
getNullablesibling method instead.");
| 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)); |
There was a problem hiding this comment.
maybe we can make NullableValue.NULL is public
| } | ||
|
|
||
| private static void expectNullNotAllowed(Executable exec) { | ||
| NullPointerException err = assertThrows(NullPointerException.class, exec); |
There was a problem hiding this comment.
we have IgniteTestUtils.assert* methods
| K key = entry.getKey(); | ||
| V val = entry.getValue(); | ||
|
|
||
| Objects.requireNonNull(key); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Added variable names to requireNonNull calls.
…a PoJo when field / column nullability do not match
…a PoJo when field / column nullability do not match
…a PoJo when field / column nullability do not match
…a PoJo when field / column nullability do not match
…a PoJo when field / column nullability do not match
…a PoJo when field / column nullability do not match
| /** | ||
| * Embedded client nulls in operations test. | ||
| */ | ||
| public class ItThinClientNullsEmbeddedTest extends ItThinClientNullsTest { |
There was a problem hiding this comment.
client is either thin or embedded
…a PoJo when field / column nullability do not match rename.
…a PoJo when field / column nullability do not match fix tests.
| Objects.requireNonNull(key); | ||
| Objects.requireNonNull(key, "key"); | ||
|
|
||
| return doGet(tx, key, "getNullableAsync"); |
There was a problem hiding this comment.
should it be a
| return doGet(tx, key, "getNullableAsync"); | |
| return doGet(tx, key, "getAsync"); |
?
There was a problem hiding this comment.
there are a few such places
There was a problem hiding this comment.
No, it should not. It reports what method should be used instead.
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
- 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.
Notes