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

Java: Add overloads for XADD to allow duplicate entry keys #1970

Merged
merged 15 commits into from
Jul 24, 2024

Conversation

jonathanl-bq
Copy link
Collaborator

@jonathanl-bq jonathanl-bq commented Jul 17, 2024

This fixes an issue with XADD where entries passed as input could not contain duplicate keys because the exposed API only took Maps.

@jonathanl-bq jonathanl-bq marked this pull request as ready for review July 18, 2024 00:28
@jonathanl-bq jonathanl-bq requested a review from a team as a code owner July 18, 2024 00:28
@yipin-chen
Copy link
Collaborator

Please fix CI breakage.

@jonathanl-bq
Copy link
Collaborator Author

There's a pretty sizable number of SpotBugs issues causing CI to fail that are unrelated to my changes here. I think I'll put the fixes for those in a separate PR.

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add changelog and fix tests

@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label Jul 18, 2024
Signed-off-by: Jonathan Louie <[email protected]>
@@ -896,7 +901,8 @@ private static Object[] streamCommands(BaseTransaction<?> transaction) {
"0-1", // xadd(streamKey1, Map.of("field1", "value1"), ... .id("0-1").build());
"0-2", // xadd(streamKey1, Map.of("field2", "value2"), ... .id("0-2").build());
"0-3", // xadd(streamKey1, Map.of("field3", "value3"), ... .id("0-3").build());
3L, // xlen(streamKey1)
"0-4", // xadd(streamKey4, new String[][] {{"field4", "value4"}, {"field4", "value5"}}), ... .id("0-4").build());
3L, // xlen(streamKey4)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the xlen above is still passing streamKey1 (line 831)

Suggested change
3L, // xlen(streamKey4)
3L, // xlen(streamKey1)

CHANGELOG.md Show resolved Hide resolved
Comment on lines 5781 to 5793
String timestamp = "1721748920942-0";
String[][] entry = new String[][] {{field, foo1}, {field, bar1}};
assertEquals(
timestamp,
client
.xadd(key, entry)
.get());

// get everything from the stream
Map<String, String[][]> result = client.xrange(key, InfRangeBound.MIN, InfRangeBound.MAX).get();
assertEquals(1, result.size());
String[][] actualEntry = result.get(timestamp);
assertDeepEquals(entry, actualEntry);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding to Yury's comment, I don't think this will pass as is, but you can modify it to get it working like this:

Suggested change
String timestamp = "1721748920942-0";
String[][] entry = new String[][] {{field, foo1}, {field, bar1}};
assertEquals(
timestamp,
client
.xadd(key, entry)
.get());
// get everything from the stream
Map<String, String[][]> result = client.xrange(key, InfRangeBound.MIN, InfRangeBound.MAX).get();
assertEquals(1, result.size());
String[][] actualEntry = result.get(timestamp);
assertDeepEquals(entry, actualEntry);
String[][] entry = new String[][] {{field, foo1}, {field, bar1}};
String streamId = client.xadd(key, entry).get());
// get everything from the stream
Map<String, String[][]> result = client.xrange(key, InfRangeBound.MIN, InfRangeBound.MAX).get();
assertEquals(1, result.size());
String[][] actualEntry = result.get(streamId);
assertDeepEquals(entry, actualEntry);

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spotless hates you

Signed-off-by: Jonathan Louie <[email protected]>
@jonathanl-bq jonathanl-bq merged commit be0363f into valkey-io:main Jul 24, 2024
6 checks passed
@jonathanl-bq jonathanl-bq deleted the java/integ_lotjonat_xadd_bug branch July 24, 2024 04:39
@asafpamzn asafpamzn mentioned this pull request Aug 11, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants