pkg/encoding: yaml.Validate doesn't handle disjunctions #2741
Labels
Comments
jpluscplusm
changed the title
jpluscplusm
changed the title
|
Verified that this is still a bug today, with and without the new evaluator experiment. |
cueckoo
pushed a commit
that referenced
this issue
May 10, 2024
The json/yaml Validate APIs currently do not correctly handle disjunctions. This CL adds test cases displaying false negatives returned by these functions. For #2741. Signed-off-by: Noam Dolovich <[email protected]> Change-Id: I10de43a09883e770c42d2499cef7ae29b3593b19
cueckoo
pushed a commit
that referenced
this issue
May 10, 2024
When calling builtin functions, there are two incorrect behaviors:
1. The evaluator evaluates each argument into an adt.Value. The
method currently used attempts to resolve all disjunctions,
so that if the following is provided:
yaml.Validate("a", "a" | "b")
the following error is returned:
unresolved disjunction "a" | "b"
This is incorrect as in CUE, unifying "a" with `"a" | "b"`
should result in "a" and not an error.
2. builtins require that all arguments are concrete. This is fine
in many cases, but not all - for example in `yaml.Validate`, it
should be fine to pass in a disjunction such as `"a" | "b"`.
To fix this, we:
- delegate the requirement to evaluate disjunctions to the builtin
functions, and do not attempt to resolve default values before
calling the builtin.
- add a function for allowing builtins to evaluate their arguments
as "non-concrete", and use it in the `yaml.Validate` and
`json.Validate` builtins.
This fixes incorrect failures when passing a disjunction to
`yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that mostly fixes #2741 but does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
May 10, 2024
When calling builtin functions, there are two incorrect behaviors:
1. The evaluator evaluates each argument into an adt.Value. The
method currently used attempts to resolve all disjunctions,
so that if the following is provided:
yaml.Validate("a", "a" | "b")
the following error is returned:
unresolved disjunction "a" | "b"
This is incorrect as in CUE, unifying "a" with `"a" | "b"`
should result in "a" and not an error.
2. builtins require that all arguments are concrete. This is fine
in many cases, but not all - for example in `yaml.Validate`, it
should be fine to pass in a disjunction such as `"a" | "b"`.
To fix this, we:
- delegate the requirement to evaluate disjunctions to the builtin
functions, and do not attempt to resolve default values before
calling the builtin.
- add a function for allowing builtins to evaluate their arguments
as "non-concrete", and use it in the `yaml.Validate` and
`json.Validate` builtins.
This fixes incorrect failures when passing a disjunction to
`yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that mostly fixes #2741 but does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
May 10, 2024
When calling builtin functions, there are two incorrect behaviors:
1. The evaluator evaluates each argument into an adt.Value. The
method currently used attempts to resolve all disjunctions,
so that if the following is provided:
yaml.Validate("a", "a" | "b")
the following error is returned:
unresolved disjunction "a" | "b"
This is incorrect as in CUE, unifying "a" with `"a" | "b"`
should result in "a" and not an error.
2. builtins require that all arguments are concrete. This is fine
in many cases, but not all - for example in `yaml.Validate`, it
should be fine to pass in a disjunction such as `"a" | "b"`.
To fix this, we:
- delegate the requirement to evaluate disjunctions to the builtin
functions, and do not attempt to resolve default values before
calling the builtin.
- add a function for allowing builtins to evaluate their arguments
as "non-concrete", and use it in the `yaml.Validate` and
`json.Validate` builtins.
This fixes incorrect failures when passing a disjunction to
`yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
May 10, 2024
The json/yaml Validate APIs currently do not correctly handle disjunctions. This CL adds test cases displaying false negatives returned by these functions. For #2741. Signed-off-by: Noam Dolovich <[email protected]> Change-Id: I10de43a09883e770c42d2499cef7ae29b3593b19 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194424 Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]>
cueckoo
pushed a commit
that referenced
this issue
May 13, 2024
When calling builtin functions, there are two incorrect behaviors:
1. The evaluator evaluates each argument into an adt.Value. The
method currently used attempts to resolve all disjunctions,
so that if the following is provided:
yaml.Validate("a", "a" | "b")
the following error is returned:
unresolved disjunction "a" | "b"
This is incorrect as in CUE, unifying "a" with `"a" | "b"`
should result in "a" and not an error.
2. all builtins fail if their arguments are not concrete. This is
fine in many cases, but not all - for example in `yaml.Validate`, it
should be fine to pass in the following (which all currently fail):
- an unresolved disjunction such as `"a" | "b"`
- a non-concrete value such as `string`
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
May 13, 2024
When calling builtin functions, an error is returned for each argument
that is:
- non-concrete (i.e. builtin(`string`))
- an unresolved disjunction (i.e. builtin("a" | "b"))
In addition, all default values are resolved, so that `builtin(1 | *2)`
will be called as `builtin(2)`, losing important data in some cases
where the disjunction could be resolved to `1`.
Some examples in which this behavior is erroneous:
- `yaml.Validate("a", "a" | "b")` (results in an "unresolved
disjunction" error)
- `yaml.Validate("a", string)` (results in a non-concrete value error)
- `yaml.Validate("a", *int | string)` (even if non-concrete values
were allowed, this would result in false because the default value
`int` would be resolved in place of the disjunction, and 'a' is
not an int)
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
May 17, 2024
When calling builtin functions, an error is returned for each argument
that is:
- non-concrete (i.e. builtin(`string`))
- an unresolved disjunction (i.e. builtin("a" | "b"))
In addition, all default values are resolved, so that `builtin(1 | *2)`
will be called as `builtin(2)`, losing important data in some cases
where the disjunction could be resolved to `1`.
Some examples in which this behavior is erroneous:
- `yaml.Validate("a", "a" | "b")` (results in an "unresolved
disjunction" error)
- `yaml.Validate("a", string)` (results in a non-concrete value error)
- `yaml.Validate("a", *int | string)` (even if non-concrete values
were allowed, this would result in false because the default value
`int` would be resolved in place of the disjunction, and 'a' is
not an int)
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
May 17, 2024
When calling builtin functions, an error is returned for each argument
that is:
- non-concrete (i.e. builtin(`string`))
- an unresolved disjunction (i.e. builtin("a" | "b"))
In addition, all default values are resolved, so that `builtin(1 | *2)`
will be called as `builtin(2)`, losing important data in some cases
where the disjunction could be resolved to `1`.
Some examples in which this behavior is erroneous:
- `yaml.Validate("a", "a" | "b")` (results in an "unresolved
disjunction" error)
- `yaml.Validate("a", string)` (results in a non-concrete value error)
- `yaml.Validate("a", *int | string)` (even if non-concrete values
were allowed, this would result in false because the default value
`int` would be resolved in place of the disjunction, and 'a' is
not an int)
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
|
Noting per a conversation offline that this this particular example (of oneofs) is intrinsically linked to #3165. |
cueckoo
pushed a commit
that referenced
this issue
Jun 7, 2024
When calling builtin functions, an error is returned for each argument
that is:
- non-concrete (i.e. builtin(`string`))
- an unresolved disjunction (i.e. builtin("a" | "b"))
In addition, all default values are resolved, so that `builtin(1 | *2)`
will be called as `builtin(2)`, losing important data in some cases
where the disjunction could be resolved to `1`.
Some examples in which this behavior is erroneous:
- `yaml.Validate("a", "a" | "b")` (results in an "unresolved
disjunction" error)
- `yaml.Validate("a", string)` (results in a non-concrete value error)
- `yaml.Validate("a", *int | string)` (even if non-concrete values
were allowed, this would result in false because the default value
`int` would be resolved in place of the disjunction, and 'a' is
not an int)
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
Jun 10, 2024
When calling builtin functions, an error is returned for each argument
that is:
- non-concrete (i.e. builtin(`string`))
- an unresolved disjunction (i.e. builtin("a" | "b"))
In addition, all default values are resolved, so that `builtin(1 | *2)`
will be called as `builtin(2)`, losing important data in some cases
where the disjunction could be resolved to `1`.
Some examples in which this behavior is erroneous:
- `yaml.Validate("a", "a" | "b")` (results in an "unresolved
disjunction" error)
- `yaml.Validate("a", string)` (results in a non-concrete value error)
- `yaml.Validate("a", *int | string)` (even if non-concrete values
were allowed, this would result in false because the default value
`int` would be resolved in place of the disjunction, and 'a' is
not an int)
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
Jun 10, 2024
When calling builtin functions, an error is returned for each argument
that is:
- non-concrete (i.e. builtin(`string`))
- an unresolved disjunction (i.e. builtin("a" | "b"))
In addition, all default values are resolved, so that `builtin(1 | *2)`
will be called as `builtin(2)`, losing important data in some cases
where the disjunction could be resolved to `1`.
Some examples in which this behavior is erroneous:
- `yaml.Validate("a", "a" | "b")` (results in an "unresolved
disjunction" error)
- `yaml.Validate("a", string)` (results in a non-concrete value error)
- `yaml.Validate("a", *int | string)` (even if non-concrete values
were allowed, this would result in false because the default value
`int` would be resolved in place of the disjunction, and 'a' is
not an int)
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
cueckoo
pushed a commit
that referenced
this issue
Aug 21, 2024
By default, arguments to builtins are checked for concreteness. It will be too much work, not to mention brittle, to change this at this moment, as it would require functions to check the result of the arguments for errors whereas before they did not. Instead, if a function now wants arguments to be able to be non-concrete, it should do so by explicitly setting the NonConcrete flag. It is then subsequently responsible for checking all return arguments. A bit of the non-concrete support is added here, but it will be used and tested in a followup CL. Otherwise, these changes should not result in any test changes. Issue #2741 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: I7a61fb93a6976b0b9b9118507ae41acb7fdd2b05 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1199684 Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]>
cueckoo
pushed a commit
that referenced
this issue
Aug 21, 2024
These tests should pass after yaml validation tests accept schema. Issue #2741 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: Ib0535340dcc7c28c06a3e36b86cab6412ea0a972 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1199708 Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]>
cueckoo
pushed a commit
that referenced
this issue
Aug 21, 2024
By default, when calling builtin functions, an error is returned
for each argument that is:
- non-concrete (i.e. builtin(`string`))
- an unresolved disjunction (i.e. builtin("a" | "b"))
In addition, all default values are resolved, so that `builtin(1 | *2)`
will be called as `builtin(2)`, losing important data in some cases
where the disjunction could be resolved to `1`.
Some examples in which this behavior is erroneous:
- `yaml.Validate("a", "a" | "b")` (results in an "unresolved
disjunction" error)
- `yaml.Validate("a", string)` (results in a non-concrete value error)
- `yaml.Validate("a", *int | string)` (even if non-concrete values
were allowed, this would result in false because the default value
`int` would be resolved in place of the disjunction, and 'a' is
not an int)
To fix this, we delegate the responsibility to evaluate disjunctions
and resolve default values to the builtin. This way each builtin can
decide whether its arguments should be concrete, and whether
disjunctions/defaults should be resolved.
Note: these are further original comments from Noam. The corresponding
code has been hoisted to a separate CL
A new CallCtxt.Schema method is added which, as opposed to
CallCtxt.Value, does not resolve arguments to their default values /
enforce concreteness.
All stdlib function registrations, located in pkg.go files, are
auto-generated based on the public functions in the corresponding
stdlib packages. Because of this, a new `internal.pkg.Schema` type
is added, so that stdlib functions that need to use CallCtxt.Schema,
can declare a parameter astype pkg.Schema.
This fixes incorrect failures when passing non-concrete values and
unresolved disjunctions to `yaml.Validate` and `json.Validate`.
A test is added that only works when the new evaluator is enabled. This
is because the old evaluator evaluated the following CUE as incomplete:
{a: 1} & ({a!: int} | {b!: int})
Which is incorect because the required fields should "force" the
expression to evaluate to `{a: 1}`.
Note that this does not yet fix cases like:
yaml.Validate("a: 1", close({a: int}) | close({b: int}))
This will require further investigation to see why the "closedness" of
the disjuncts is seemingly lost during the evaluation, resulting in a
non-concrete unification of the yaml with the disjunction - and a false
result.
Updates #2741.
Signed-off-by: Noam Dolovich <[email protected]>
Change-Id: Id34b5ada4704df0e43ff9ea33ad47d631af9ba74
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194425
Unity-Result: CUE porcuepine <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What version of CUE are you using (
cue version)?$ cue version cue version v0.7.0 go version go1.21.5 -buildmode exe -compiler gc -trimpath true CGO_ENABLED 0 GOARCH amd64 GOOS linux GOAMD64 v1Does this issue reproduce with the latest stable release?
Yes, 0.7.0.
What did you do?
As per https://cuelang.slack.com/archives/C012UU8B72M/p1703146006424709:
None of the 3 attempts in the repro validates the inline YAML.
What did you expect to see?
A passing test, or at least /one/ of the attempts succeeding (i.e. fewer than 3 failures in the
testscript --continueoutput).What did you see instead?
The text was updated successfully, but these errors were encountered: