Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix testNullValuesFromMapLoaderAreNotInsertedIntoMap failure #26210

Closed
wants to merge 1 commit into from

Conversation

alyokaz
Copy link

@alyokaz alyokaz commented Jan 9, 2024

Fixes failure to throw a NullPointerException when a null value from a MapLoader is inserted into a Map.

The issue appears to been have introduced in commit 7ec4749 in which the null value is chosen to be tolerated as normal rather then exceptional.

I've reused the code DefaultRecordStore as this was the check in the original flow of the code. The partitionId check is superfluous here, however.

Fixes #26184

…ap failure

Fixes failure to throw a NullPointerException when a null value from a MapLoader
is inserted into a Map.

The issue appears to been have introduced in commit (#73b9460) in which the null
value is chosen to be tolerated as normal rather then exceptional.

fixes (hazelcast#26184)
@hz-devops-test hz-devops-test added the Source: Community PR or issue was opened by a community user label Jan 9, 2024
@devOpsHazelcast
Copy link
Collaborator

devOpsHazelcast commented Jan 9, 2024

CLA assistant check
All committers have signed the CLA.

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

2 similar comments
@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Can one of the admins verify this patch?

@devOpsHazelcast
Copy link
Collaborator

Internal PR hazelcast/hazelcast-mono#359
Internal message only. Nothing to see here, move along

@vbekiaris
Copy link
Contributor

@alyokaz thanks for the pull request! @ahmetmircik can you please have a look?

@alyokaz
Copy link
Author

alyokaz commented Jan 10, 2024

Just wanted to point out that the Refactor deals with null values being added by passing over them and not adding them to the map both when a key has a null value and in the case of a non-existent key. This is why the NPE wasn't being thrown, however, the fix will be over-zealous in the latter case and should probably be made conditional on the existence of the key if we want to allow for both behaviours.

@alyokaz
Copy link
Author

alyokaz commented Jan 11, 2024

Thinking about this further, the fix does fulfil the IMap.loadAll() contract that an NPE should be thrown if any key or value returned by MapLoader.loadAll() is null. However, this might be considered inconsistent with the behaviour of get() witch returns null if the the key does not exist as opposed to throwing an NPE. getAll() calls loadAll() and so an NPE is thrown there as well.

@ahmetmircik
Copy link
Member

Hi, just have a look at this.

IMap#getAll behavior in master branch is inline with vanilla IMap#get, while they never let null values, they also don’t throw NPE exception when map-loader has no matching value for a key. We need to fix the test code, getAll’s implementation code seems ok.
So to recap, IMap#getAll only returns matching values and when there is no matching value, it silently ignores it as in ICache#getAll.

@AleksPeychev AleksPeychev added Team: Core Source: Internal PR or issue was opened by an employee and removed Team: Core Source: Internal PR or issue was opened by an employee Source: Community PR or issue was opened by a community user labels Feb 13, 2024
@devOpsHazelcast
Copy link
Collaborator

This pull request has been closed because it was already closed as https://github.com/hazelcast/hazelcast-mono/pull/359

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

com.hazelcast.client.map.ClientMapStoreTest.testNullValuesFromMapLoaderAreNotInsertedIntoMap [HZ-4257]
6 participants