Skip to content

Fix(#12318): Fixes a bug in evaluation of JSONPath for wildcard and s… #12366

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
Merged
merged 5 commits into from
Apr 2, 2025
Merged

Fix(#12318): Fixes a bug in evaluation of JSONPath for wildcard and s… #12366

merged 5 commits into from
Apr 2, 2025

Conversation

marcodallasanta
Copy link
Contributor

Motivation

This is an attempt to fix issue #12318

Changes

Changes jsonpath.py.
When no matches, then it checks if the pattern of the JSONPath contains an array slice or wildcard, if so it gracefully returns an empty array instead of failing with NoSuchJsonPathError.

There's an exception though, if the JSONPath starts with an array slice syntax than it should fail.

Testing

There's a parameterised test for each of the pattern I identified to be problematic, but below there's a little script if it can help in diagnose issues

# 1. start docker
docker run --rm -d --name "localstack-main" -p 4566:4566 localstack/localstack

# 2. create a state machine - Change the JsonPath as you like
stateMachineArn=$(awslocal stepfunctions create-state-machine --name "test" --definition '{
  "StartAt": "Pass",
  "States": {
    "Pass": {
      "Type": "Pass",
      "End": true,
      "Parameters": {
        "types.$": "$.items[*]"
      }
    }
  }
}'  --role-arn "dummy" | jq -r ".stateMachineArn");

# 3. create an execution 
executionArn=$(awslocal stepfunctions start-execution --state-machine-arn "$stateMachineArn" | jq -r ".executionArn");

# 4. describe the execution (jq for syntax highlight)
awslocal stepfunctions describe-execution --execution-arn "$executionArn" | jq "."

@marcodallasanta
Fix(localstack#12318): Fixes a bug in evaluation of JSONPath for wild…
e326730 
…card and slices patterns executed on a missing array.
localstack-bot
localstack-bot reviewed Mar 11, 2025
View reviewed changes
Copy link
Collaborator

@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@localstack-bot
Copy link
Collaborator

localstack-bot commented Mar 11, 2025 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@marcodallasanta marcodallasanta mentioned this pull request Mar 11, 2025
Closed
1 task
@marcodallasanta
Copy link
Contributor Author

I wanted to add a label as semver: patch, but looks like I don't have permissions.

@marcodallasanta
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@marcodallasanta
Copy link
Contributor Author

recheck

localstack-bot added a commit that referenced this pull request Mar 11, 2025
@localstack-bot
@marcodallasanta has signed the CLA in #12366
31d83ab
@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Mar 12, 2025
@k-a-il k-a-il added this to the Playground milestone Mar 24, 2025
MEPalma
MEPalma reviewed Apr 1, 2025
View reviewed changes
Copy link
Contributor

@MEPalma MEPalma left a comment

Choose a reason for hiding this comment

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

Thank you @marcodallasanta for contributing to LocalStack and providing a solution for #12318! Your proposed approach will definitely help unblock this scenario.

I just have a minor refinement request regarding the snapshot tests. Specifically, could you please update your PR to ensure that only the snapshot information relevant to your new test case is included? Additionally, please make sure to incorporate your test case changes into the validation file.

For reproducibility, kindly avoid manually editing the generated files. Instead, use transformers within the sfn_snapshot instance if necessary (though this should not be required in this case).

If you’d prefer, I’d also be happy to take care of these changes—just let me know!

Thanks again, looking forward to merging this soon!

@marcodallasanta
Copy link
Contributor Author

Thank you @MEPalma, will try myself just to gain a more experience with the repo in case I will contribute again, but if it takes too long I will leave it to you.

@marcodallasanta
Copy link
Contributor Author

Hey @MEPalma, I did what you suggested above:

  • reverted and updated only with the relevant tests the snapshot file.
  • updated the validation.json file.

Let me know if there's anything else that needs to be done before merging.

MEPalma
MEPalma approved these changes Apr 2, 2025
View reviewed changes
Copy link
Contributor

@MEPalma MEPalma left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to address the comments, and once again, for your valuable contribution to the project!

@MEPalma MEPalma merged commit 4447e30 into localstack:master Apr 2, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@localstack-bot localstack-bot localstack-bot left review comments

@MEPalma MEPalma MEPalma approved these changes

@joe4dev joe4dev Awaiting requested review from joe4dev joe4dev is a code owner

@gregfurman gregfurman Awaiting requested review from gregfurman gregfurman is a code owner

Assignees
No one assigned
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
Playground
Development

Successfully merging this pull request may close these issues.
4 participants
@marcodallasanta @localstack-bot @MEPalma @k-a-il