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 JSONPath inconsistencies #1266

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix JSONPath inconsistencies #1266

wants to merge 7 commits into from

Conversation

N3XT14
Copy link

@N3XT14 N3XT14 commented Nov 11, 2024

@JyotinderSingh.

Sorry for the delay. I was busy with something urgent at work and couldn't give enough time.

Have a look at this PR. It's not completed yet but good portion of it is completed. Unfortunately, I am unable to find how to enable the resp3 protocol.

Also, in the Data Link shared the multi path examples seems incorrect in the array example.

Currently, I am testing it on my end but since certain parts such as RESP3 and custom formats are pending I am not asking for a merge yet.

I wanted to also open up a discussion on certain outputs that Redis returns such as for example "accessing negative index on out of bounds for an arrray example", etc.

Should we be following that as I believe we should instead return error for such cases instead of nil or the way redis handles.

Fixes #1002

@JyotinderSingh JyotinderSingh changed the title Issue 1002 fix JSONPath inconsistencies Nov 11, 2024
@JyotinderSingh
Copy link
Collaborator

We're not looking to support RESP3 at this stage. Let's stick with RESP2.

Is RESP3 support a blocker in any way?

@N3XT14
Copy link
Author

N3XT14 commented Nov 11, 2024

Nope, definitely not a blocker.

@JyotinderSingh
Copy link
Collaborator

Nope, definitely not a blocker.

In that case we can remove the related code and limit this PR to just the jsonpath changes.

@JyotinderSingh
Copy link
Collaborator

Looks like the tests and linter are failing, could you please take a look? Also, is this PR ready for review?

@N3XT14
Copy link
Author

N3XT14 commented Nov 20, 2024

Looks like the tests and linter are failing, could you please take a look? Also, is this PR ready for review?

Hi Yes,

Apologies, I was traveling the entire week so couldn't push.

I have pushed the latest changes. Test cases required changes in output structure mainly. I ran linter on my local before pushing it through some warnings and error at other parts of the code. I am not sure why it didn't caught attention earlier. Currently, GitHub actions is seeking approval

@JyotinderSingh
Copy link
Collaborator

Looks like the tests and linter are failing, could you please take a look? Also, is this PR ready for review?

Hi Yes,

Apologies, I was traveling the entire week so couldn't push.

I have pushed the latest changes. Test cases required changes in output structure mainly. I ran linter on my local before pushing it through some warnings and error at other parts of the code. I am not sure why it didn't caught attention earlier. Currently, GitHub actions is seeking approval

Thanks for the update! Since you've never pushed a commit into the repository before your PRs require a manual github actions run.
I've started the CI for this change.

@JyotinderSingh
Copy link
Collaborator

Looks like there are some test failure, could you please take a look.

@N3XT14
Copy link
Author

N3XT14 commented Nov 21, 2024

Looks like there are some test failure, could you please take a look.

There are two test cases failing:

  1. JSON.MGET in resp: This issue likely stems from a mismatch in the output interface, which occurred during migration. Since my code doesn't directly impact the MGET test cases, I believe the failure is related to the migration process rather than any recent changes in the code.

  2. JSON.GET for UnsupportedJSONPathOperations (including regex): Previously, regex was not supported in the JSONPath operations, but now it is. I have updated the test cases to reflect this change. However, another issue arises from the parsing logic in the ParseCommand function, which is located in the parsecommand.go file. The function currently only considers double quotes to determine if subsequent characters belong to the same path. In earlier test cases, single quotes were used, causing the parser to incorrectly treat them as separate paths. I have updated the test cases to use double quotes, but we need to improve the handling of quotes in the ParseCommand function to avoid similar issues in the future.

@lucifercr07
Copy link
Contributor

@N3XT14 any updates on this? Please let us know if any help required on this.

@N3XT14
Copy link
Author

N3XT14 commented Dec 9, 2024

Hi @lucifercr07. The implementation and test cases are working.

Latest commit: d5fa222

Furthermore, I am waiting for a response on the above two questions mentioned in previous comment.

@JyotinderSingh
Copy link
Collaborator

Hi @N3XT14, could you please rebase the PR on the latest commit? I'm wondering if some issues would automatically get fixed.

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.

Fix JSONPath inconsistencies in DiceDB
4 participants