Skip to content

Conversation

@andras-markos
Copy link
Contributor

This fixes var resolution so a default is applied only when the path is missing, not when the value is explicitly null.

Why

Per the JsonLogic docs, the second var argument is a default “for values that might be missing.” Treating null as missing deviates from that intent and leads to surprising results.

What changed

  • Introduced an internal MISSING sentinel to track “not found” during path traversal.
  • Map access now uses containsKey to differentiate an absent key from a present key with null.
  • Array access returns MISSING for out-of-bounds indices.
  • evaluate(JsonLogicVariable, …) applies the default only when the traversal yields MISSING; explicit null is returned as null.

Examples

  • {"var": ["user.age", 42]} with {"user":{"age": null}}null (no default)
  • {"var": ["items.2", "missing"]} with {"items":[10,20]}"missing"
  • {"var": ""} returns the entire data object (same instance)

Tests

Added coverage to VariableTests:

  • Missing nested key falls back to default.
  • Present leaf null returns null (no default).
  • Intermediate null or non-traversable types return null.
  • Array index in-bounds returns the element; out-of-bounds uses default.
  • Top-level numeric index over lists works.
  • Empty var key returns the original data object.

Compatibility

Behavior change for callers who relied on “default-for-null.” This aligns with the JsonLogic spec and other interpreters.

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove

Copy link
Owner

@jamsesso jamsesso left a comment

Choose a reason for hiding this comment

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

I'm happy with the code fixes but I'm not onboard with the CI changes. Those should be a separate PR. Because the two are mixed in here, I'll have to decline.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this change

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this change

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine upgrading Gradle, but can this be done in another PR?

Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as before - bundling two different changes in a PR (missing + gradle changes) isn't ideal. I don't want to discuss two changes in a single PR. the code for MISSING seems fine and can be deployed, but this is something I'd like to think about more.

@andras-markos
Copy link
Contributor Author

Sorry about that, that was not my intention.
I have opened a new PR for just the fix.
I only wanted to add the rest of the changes to the sailthru fork.
This PR can be closed.

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.

2 participants