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

cmd/cue: JSONSchema import translates a definition with nested sub-definitions as being equal to top #2699

Closed
jpluscplusm opened this issue Nov 20, 2023 · 5 comments
Labels

Comments

@jpluscplusm
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

I'm cue importing a specific JSONSchema document, and a "definition" that it contains is translated into an empty constraint, unifying it with top. (I'm filing this as a feature request as the emitted CUE isn't technically invalid or incorrect, but is merely unhelpfully unconstrained (modulo #2698, filed in parallel with this, and which /feels/ unrelated)).

The source document is wget'd in the txtar repro, instead of including it here, as it's over a thousand lines long. The emitted CUE is <25% of that, but feels too long to embed in this Issue. I'll assume you can fetch, import, and view the emitted CUE.

exec wget -q https://raw.githubusercontent.com/buildkite/pipeline-schema/6396f68d5e983e0d2acbf829c565027a4cfd69bc/schema.json
exec cue import schema.json
exec grep commonOptions: schema.cue
cmp stdout the-only-constraint.txt
-- the-only-constraint.txt --
#commonOptions: _

Describe the solution you'd like

I'd like cue import to delve deeper into the contents of commonOptions and include its constraints in the CUE that's emitted.

Describe alternatives you've considered

I've considered hand-cranking a schema.

Additional context

commonOptions contains several different fields which are used as isolated containers for constraints, referenced elsewhere in the source document. cue import appears to correctly figure this out, emitting references to contained sub-fields that should be expected to exist inside commonOptions.

IIRC @myitcv suggested the possibility that it's the "$ref" use of a field inside commonOptions (and not directly referencing a top level JSONSchema "definition" such as commonOptions itself) that's prompting cue import to emit the useless top level _ constraint and to omit the constraints contained within the definition's member fields.

@jpluscplusm jpluscplusm added FeatureRequest New feature or request Triage Requires triage/attention jsonschema / openapi labels Nov 20, 2023
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 22, 2023
Here's a new piece of content, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

Closes cue-labs#21
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 22, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

Closes cue-labs#21
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 22, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

Closes cue-labs#21
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 22, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

Closes cue-labs#21

Signed-off-by: Jonathan Matthews <[email protected]>
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 22, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

This is related to cue-labs#21 but doesn't /close/ it, as that issue tracks a
cue-by-example for Buildkite's "dynamic" pipeline mode, which this guide
doesn't address.

Signed-off-by: Jonathan Matthews <[email protected]>
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 23, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

This is related to cue-labs#21 but doesn't /close/ it, as that issue tracks a
cue-by-example for Buildkite's "dynamic" pipeline mode, which this guide
doesn't address.

Signed-off-by: Jonathan Matthews <[email protected]>
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 29, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

This is related to cue-labs#21 but doesn't /close/ it, as that issue tracks a
cue-by-example for Buildkite's "dynamic" pipeline mode, which this guide
doesn't address.

Signed-off-by: Jonathan Matthews <[email protected]>
jpluscplusm added a commit to jpluscplusm-forks/cue-lang.cue-by-example that referenced this issue Nov 29, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

This is related to cue-labs#21 but doesn't /close/ it, as that issue tracks a
cue-by-example for Buildkite's "dynamic" pipeline mode, which this guide
doesn't address.

Signed-off-by: Jonathan Matthews <[email protected]>
myitcv pushed a commit to cue-labs/cue-by-example that referenced this issue Nov 29, 2023
Here's a new cue-by-example, showing how to manage a set of Buildkite
pipeline files in CUE instead of YAML.

Because Buildkite (in its non-dynamic-pipeline mode) still needs to see
a YAML file serialised in the repo, the guide includes a CUE _tool that
turns the CUE back into YAML on demand.

It also includes a schema for the pipeline's representation, but this
schema is very trivial, and not hugely useful - it could do with
improving. I wasn't able to use the upstream schema
(https://github.com/buildkite/pipeline-schema) because of issues now
tracked as cue-lang/cue#2698 and cue-lang/cue#2699.

This is related to #21 but doesn't /close/ it, as that issue tracks a
cue-by-example for Buildkite's "dynamic" pipeline mode, which this guide
doesn't address.

Signed-off-by: Jonathan Matthews <[email protected]>
@myitcv myitcv removed the Triage Requires triage/attention label Oct 31, 2024
@rogpeppe
Copy link
Member

The reason for this is that definitions should contain a list of schemas, but commonOptions is not a schema: it's a map that contains schemas. In general, referring into non-schemas is (not necessarily supported](https://json-schema.org/draft/2020-12/json-schema-core#section-9.4.2), and if we see an arbitrary map entry that's not a keyword, it should be ignored.

That said, the new refactored reference implementation does generate schemas for internal structure that's been referred to directly within the schema itself, and this does indeed in this case result in the definitions in commonOptions being defined, albeit in a private field. Initially it won't be possible to change that on the command line, but it certainly could/will be in the future.

@myitcv
Copy link
Member

myitcv commented Dec 13, 2024

@rogpeppe - could/should we be suggesting an upstream fix in this case?

@rogpeppe
Copy link
Member

@rogpeppe - could/should we be suggesting an upstream fix in this case?

Yup. As it turns out, I've actually done that already - I just forgot that that fix was for this particular schema :)

cueckoo pushed a commit that referenced this issue Dec 17, 2024
This refactors references to use a general system
that maps references through a new `MapRef` function
that supercedes the existing `MapURL` and `Map` functions.
To support this, we use the new `structBuilder` type
to build up the final syntax.

This fixes a bunch of existing reported and unreported issues, including:
- json pointer escaping: JSON pointers were not previously
escaped and unescaped correctly.
- references into internal structure: `$ref` references can now
reference arbitrary internal structure inside the schema
being extracted, including nodes that aren't actually schemas.
- better doc comments: the outer-level doc comment is
now correctly preserved in all circumstances

There are inevitably some changes in the form of the generated schemas:
- field ordering of definitions is now always lexical
- some comments move to new (better) locations
- attribute placement also moves to a (better) location
- by default, only top level `$defs` members are exported
as public definitions.

The last issue could be considered a backward incompatible
change, but in practice
- nested definitions are rare
- the nested definitions were not easily accessible anyway
in most cases (e.g. when inside a property or other expression)
- the new `MapRef` feature can be used to change the
location of any schema, including these.

As yet, the `MapRef` functionality as provided to the API
is not tested other than with `DefaultMapRef`, and the
`DefineSchemas` callback is not wired up. This will
land in a subsequent CL: in the meantime, what we've
got here seems sufficient as an intermediate step.

Fixes #3593
Fixes #3548
Updates #2699
Fixes #2287
Fixes #390

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: Icfcff6e3d9f1d09f0418ddd493e01beb78045d59
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1205706
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
@myitcv
Copy link
Member

myitcv commented Dec 17, 2024

@jpluscplusm @rogpeppe - please can you confirm if there is anything outstanding here therefore?

@rogpeppe
Copy link
Member

The only outstanding thing here is that there's currently no way on the command-line to specify that all subschemas are made available as part of the API. It should be possible to work around that shortcoming in this particular case by adding some code in the same package, but I've created #3628 to track the more general request.

I'll close this as fixed for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants