Skip to content

fix: has() on non-container types returns error instead of false #1298

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

Open
kodareef5 wants to merge 1 commit into
base: master
Choose a base branch
from
+24 −1
Open

fix: has() on non-container types returns error instead of false #1298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cel/cel_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2869,6 +2869,23 @@ func TestOptionalValuesEval(t *testing.T) {
expr: `has({?'foo': optional.none()}.foo.value)`,
out: "no such key: foo",
},
// has() on non-container types errors instead of returning false.
{
expr: `has(dyn(42).field)`,
out: "no such key: field",
},
{
expr: `has(dyn('hello').field)`,
out: "no such key: field",
},
{
expr: `has(dyn(true).field)`,
out: "no such key: field",
},
{
expr: `has(dyn(null).field)`,
out: "no such key: field",
},
{
expr: `{}.?invalid`,
out: types.OptionalNone,
Expand Down
8 changes: 7 additions & 1 deletion interpreter/attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1419,7 +1419,13 @@ func refQualify(adapter types.Adapter, obj any, idx ref.Val, presenceTest, prese
return val, true, nil
default:
if presenceTest && !errorOnBadPresenceTest {
return nil, false, nil
// Optional values and optional field selection (presenceOnly=false)
// report not-present for non-container types. has() macro
// (presenceOnly=true) on non-optional primitives falls through
// to missingKey to avoid silent false on bad presence tests.
if _, isOpt := celVal.(*types.Optional); isOpt || !presenceOnly {

@TristonianJones TristonianJones Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you add a test case that demonstrates the behavior addressed by this change? The presenceOnly flag is just an optimization to indicate that the return value isn't necessary - only the presence bit.

@kodareef5 kodareef5 Apr 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added test cases for has() on int, string, bool, and null via dyn():

{expr: `has(dyn(42).field)`, out: "no such key: field"},
{expr: `has(dyn('hello').field)`, out: "no such key: field"},
{expr: `has(dyn(true).field)`, out: "no such key: field"},
{expr: `has(dyn(null).field)`, out: "no such key: field"},

On the presenceOnly flag -- you're right that it's an optimization. In this context it serves as the only available signal that distinguishes has() (where testOnlyQualifier always passes presenceOnly=true) from ?. optional select (where applyQualifiers passes presenceOnly=false). The ?. path needs to keep returning not-present on non-container types to preserve optional.none() chaining, while has() on a raw primitive like dyn(42).field should error rather than silently returning false.

I initially tried dropping the presenceOnly check and erroring for both paths, but that breaks the ?. tests ({0: dyn(0)}[?0].?invalid expects optional.none(), not an error). Happy to restructure if you'd prefer a different approach, e.g. threading a separate flag through the qualifier chain.

@TristonianJones TristonianJones Apr 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These cases error properly though when EnableErrorOnBadPresenceTest(true) is enabled, so I think that's why I'm not clear on what the issue is that you're trying to address.

return nil, false, nil
}
}
return nil, false, missingKey(idx)
}
Expand Down