Skip to content

feat: JSON.MERGE plus unit and integration tests #95

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.

Sign up for GitHub

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

Jump to bottom
Open
patrickn2 wants to merge 11 commits into
base: unstable
Choose a base branch
from
Open

feat: JSON.MERGE plus unit and integration tests #95

patrickn2 wants to merge 11 commits into from

Conversation

@patrickn2
Copy link

@patrickn2 patrickn2 commented Dec 30, 2025 •
edited
Loading

JSON.MERGE Command
Followed instructions here https://redis.io/docs/latest/commands/json.merge/

@patrickn2 patrickn2 mentioned this pull request Dec 30, 2025
Open
@roshkhatri
Copy link
Member

roshkhatri commented Dec 30, 2025 •
edited
Loading

Can you please add --sign off to the commits?: https://github.com/valkey-io/valkey-json/pull/95/checks?check_run_id=59176701849

@patrickn2
Copy link
Author

patrickn2 commented Dec 30, 2025

Can you please add --sign off to the commits?: https://github.com/valkey-io/valkey-json/pull/95/checks?check_run_id=59176701849

I guess I made a mess trying to fix it
Could you take a look?

@roshkhatri
Copy link
Member

roshkhatri commented Dec 30, 2025

Can you please add --sign off to the commits?: https://github.com/valkey-io/valkey-json/pull/95/checks?check_run_id=59176701849

I guess I made a mess trying to fix it Could you take a look?

Yeah, its okay , you can just run these commands and it should be fine:

git rebase HEAD~3 --signoff
git push --force-with-lease origin feat--JSON.MERGE-Command

@patrickn2 patrickn2 force-pushed the feat--JSON.MERGE-Command branch from 9301a02 to 3bb9a5d Compare December 30, 2025 22:18
@patrickn2
Copy link
Author

patrickn2 commented Dec 30, 2025

Can you please add --sign off to the commits?: https://github.com/valkey-io/valkey-json/pull/95/checks?check_run_id=59176701849

I guess I made a mess trying to fix it Could you take a look?

Yeah, its okay , you can just run these commands and it should be fine:

git rebase HEAD~3 --signoff
git push --force-with-lease origin feat--JSON.MERGE-Command

Done

roshkhatri
roshkhatri reviewed Jan 6, 2026
View reviewed changes
Copy link
Member

@roshkhatri roshkhatri left a comment

Choose a reason for hiding this comment

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

Great PR, just some suggestions and may need to add more tests

tst/integration/conftest.py
@@ -0,0 +1,128 @@
"""
Local conftest.py to apply fixes to the test infrastructure.
Copy link
Member

@roshkhatri roshkhatri Jan 6, 2026

Choose a reason for hiding this comment

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

Do we need to upstream this to https://github.com/valkey-io/valkey-test-framework?

Copy link
Author

@patrickn2 patrickn2 Jan 6, 2026

Choose a reason for hiding this comment

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

Not sure
At least here in my machine, integration tests were flaky, freezing and other weird behaviors
If in yours is running fine we can ignore it.

Copy link
Author

@patrickn2 patrickn2 Jan 6, 2026

Choose a reason for hiding this comment

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

@roshkhatri do I have to do anything else?

Copy link
Member

@roshkhatri roshkhatri Jan 6, 2026

Choose a reason for hiding this comment

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

I want to run this by @KarthikSubbarao to see if we can add these fixes to the test infrastructure rather than keeping these local

Copy link
Author

@patrickn2 patrickn2 Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

ok thanks
Let me know if you need anything else.

@patrickn2 patrickn2 force-pushed the feat--JSON.MERGE-Command branch from 7019ec1 to decee1d Compare January 6, 2026 14:00
roshkhatri and others added 9 commits January 6, 2026 11:37
@roshkhatri @patrickn2
Revert "Simplify offline build (valkey-io#73)" (valkey-io#93)
376822b 
This reverts commit ac5a0e4.

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
feat: JSON.MERGE plus unit and integration tests Signed-off-by: Patri…
70c8d9d 
…ck Nogueira <patricknn@gmail.com>

Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
feat: Enhance merge_values function to use configurable max depth lim…
18320b6 
…it and add unit tests for depth limit scenarios

Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
refactor: Simplify JSON merge logic by consolidating path handling an…
d976f02 
…d improve memory tracking. Update unit tests for merge values.

Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
feat: Enhance merge_values function to handle empty and non-object ca…
83f5983 
…ses, and add unit tests for various merge scenarios

Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
fix: Correct parameter name in merge_values function and add unit tes…
dea9a1c 
…ts for various merge scenarios including array and object replacements

Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
feat: Add integration tests for JSON.MERGE command covering wildcards…
ac59fe8 
…, mixed updates, error handling, and no-op scenarios

Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
chore: Remove JSON.MERGE documentation files
d79a320 
Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2
chore: rebasing cmakelists and build.sh
9713e53 
Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
@patrickn2 patrickn2 force-pushed the feat--JSON.MERGE-Command branch from b53bf08 to 9713e53 Compare January 6, 2026 14:37
@patrickn2
Merge branch 'unstable' into feat--JSON.MERGE-Command
af7dc01 
Signed-off-by: Patrick Nery Nogueira <119355744+patrickn2@users.noreply.github.com>
@roshkhatri
Copy link
Member

roshkhatri commented Jan 6, 2026

The failing CI would be fixed once we merge the latest unstable into this branch

@roshkhatri
Copy link
Member

roshkhatri commented Jan 6, 2026

I am not sure though why these are failing: https://github.com/valkey-io/valkey-json/actions/runs/20278225578/job/58232842208
here the cloning into valkey is working but on this PR there seems to be some issue

roshkhatri
roshkhatri reviewed Jan 6, 2026
View reviewed changes
@patrickn2
fix: Merge conflict
154cd9b 
Signed-off-by: patrick.nogueira@fox.com <patrick.nogueira@fox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roshkhatri roshkhatri roshkhatri left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@patrickn2 @roshkhatri