Skip to content

fix: Replace falsy values with json selector#666

Merged
e-fisher merged 4 commits intomainfrom
fix/replace-falsy-values
Apr 23, 2025
Merged

fix: Replace falsy values with json selector#666
e-fisher merged 4 commits intomainfrom
fix/replace-falsy-values

Conversation

@e-fisher
Copy link
Collaborator

Description

This fixes the issue when JSON body selectors don't work on falsy properties (empty string, boolean false).

How to Test

Create a parameterization or correlation rule targeting a falsy property using JSON body selector.

Expected:
Rule matches and value replaced

Current:
Rule doesn't match

Warning

Don't rely on quickpizza for testing this because it sets incorrect content type (text/plain) and JSON selectors are not applied.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (npm run lint) and all checks pass.
  • I have run tests locally (npm test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

Related PR(s)/Issue(s)

Resolves #654

Copilot AI review requested due to automatic review settings April 16, 2025 13:31
@e-fisher e-fisher requested a review from a team as a code owner April 16, 2025 13:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the issue where JSON body selectors were not matching falsy values by updating the condition check to only return when the value is undefined.

  • Changed condition in replaceJsonBody from a falsy check to a strict undefined check.
  • Added tests to cover scenarios with empty string and boolean values in JSON bodies.
Comments suppressed due to low confidence (2)

src/rules/shared.ts:258

  • Consider adding a test case for when the JSON property is null, if null is a possible value, to ensure the intended behavior is maintained.
if (valueToReplace === undefined) return

src/rules/shared.ts:573

  • It may be beneficial to include tests for additional edge cases, such as when JSON values are explicitly null or other uncommon falsy values, to improve overall test coverage.
})

Copy link
Collaborator

@cristianoventura cristianoventura left a comment

Choose a reason for hiding this comment

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

Works well for parameterization, but I found issues when trying to extract a falsy value from a correlation.

  • I tested the correlation rule using this endpoint https://pokeapi.co/api/v2/pokemon/ditto
  • Attempt to extract abilities[0].is_hidden with no success

image

I did some debugging and found two missing places for it to work for correlation rules. We need to explicitly compare against undefined, similar to what you did in the PR.

if (extractedValue && correlationExtractionSnippet) {

@e-fisher
Copy link
Collaborator Author

@cristianoventura good catch! This is an interesting case, because usually you wouldn't want to replace all "false" value, but perhaps, with the combination of custom replacer, this could be used to have dynamic boolean in payload depending on a previous response.

Copy link
Collaborator

@cristianoventura cristianoventura left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@e-fisher e-fisher merged commit c6cd3f2 into main Apr 23, 2025
3 checks passed
@e-fisher e-fisher deleted the fix/replace-falsy-values branch April 23, 2025 06:37
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.

json replacer not working for falsy values in Parameterization

2 participants