Skip to content
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.

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

"field not allowed" error when embedding a definition inside an if comprehension #3486

Closed
coding-yogi opened this issue Oct 9, 2024 · 2 comments
Labels
closedness evaluator evalv3-win Issues resolved by switching from evalv2 to evalv3 NeedsFix

Comments

@coding-yogi
Copy link

What version of CUE are you using (cue version)?

$ cue version
v0.10.0

Does this issue reproduce with the latest stable release?

Yes

What did you do?

Trying to eval below cue config

#cluster: {
    karpenter: {
      enable: bool | *true
    }
}

_cluster: #cluster

#service_account_annotations: {
  [Name=string]: {
    service_account: {
      annotations: {
        "test": "test"
      }
    }
  }
}

if _cluster.karpenter.enable {
  #service_account_annotations:karpenter: {}

  karpenter: {
    replicas: int | *2
    #service_account_annotations.karpenter
  }
}

What did you expect to see?

I except the if condition to evaluate and generate the required karpenter struct, but it fails. If the if statement is commented, then code works fine.

I understand that #service_account_annotations:karpenter: {} is a closed struct , and making it open works.
But I am not sure why it complains about the fields of karpenter

What did you see instead?

karpenter.replicas: field not allowed:
@coding-yogi coding-yogi added NeedsInvestigation Triage Requires triage/attention labels Oct 9, 2024
@mvdan
Copy link
Member

mvdan commented Oct 9, 2024

Nicely spotted, this appears to be a bug in both the old and new evaluator. Simplified below:

env CUE_EXPERIMENT=evalv3=0
exec cue export input.cue

env CUE_EXPERIMENT=evalv3=1
exec cue export input.cue

-- input.cue --
#schema: {}

if true {
	out: {
		#schema
		extra: "foo"
	}
}

Which results in the following testscript output:

> env CUE_EXPERIMENT=evalv3=0
> exec cue export input.cue
[stderr]
out.extra: field not allowed:
    ./input.cue:1:10
    ./input.cue:3:1
    ./input.cue:5:3
    ./input.cue:6:3
[exit status 1]
FAIL: test.cue:2: unexpected command failure
> env CUE_EXPERIMENT=evalv3=1
> exec cue export input.cue
[stderr]
out.extra: field not allowed:
    ./input.cue:6:10
    ./input.cue:6:3
[exit status 1]
FAIL: test.cue:5: unexpected command failure
failed run

I know this is a bug because removing the if true conditional results in JSON output. This bug seems to be pretty similar to #3483, except that it affects the old evaluator too.

@mvdan mvdan added NeedsFix closedness and removed NeedsInvestigation Triage Requires triage/attention labels Oct 9, 2024
@mvdan mvdan changed the title Closed embedded struct impacting the parent struct "field not allowed" error when embedding a definition inside an if comprehension Oct 9, 2024
cueckoo pushed a commit that referenced this issue Oct 9, 2024
The code currently used matchPattern directly
in processComprehesionInner, which really should
use allows.

Hoist the code so it can be shared. We do so in
a separate CL to keep no-op changes separated
from material ones.

We'll fix the bug in a followup CL.

Issue #3486

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I02f743898e33c8e6fdd03fccfc9080a9b63535de
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202420
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
@cueckoo cueckoo closed this as completed in cd2107a Oct 9, 2024
@mvdan mvdan added the evalv3-win Issues resolved by switching from evalv2 to evalv3 label Oct 9, 2024
@mvdan
Copy link
Member

mvdan commented Oct 9, 2024

Note that the commit above only fixes the issue for the new evaluator, enabled via CUE_EXPERIMENT=evalv3.

vanhtuan0409 pushed a commit to anduintransaction/cue that referenced this issue Oct 15, 2024
The code currently used matchPattern directly
in processComprehesionInner, which really should
use allows.

Hoist the code so it can be shared. We do so in
a separate CL to keep no-op changes separated
from material ones.

We'll fix the bug in a followup CL.

Issue cue-lang#3486

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I02f743898e33c8e6fdd03fccfc9080a9b63535de
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202420
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
vanhtuan0409 pushed a commit to anduintransaction/cue that referenced this issue Oct 15, 2024
This consists of two fixes:

1. use allows instead of matchPattern to check for fields
   that are not covered by patterns.
2. updates the arcTypes _after_ calling allows, as
   allows needs the old values.

Fixes cue-lang#3486

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I85c04f814bf0cb67a2a57dc10205ad2a309cb61f
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202421
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closedness evaluator evalv3-win Issues resolved by switching from evalv2 to evalv3 NeedsFix
Projects
None yet
Development

No branches or pull requests

2 participants