encoding/jsonschema: pass comments to DefineSchema

This is a retry of https://cuelang.org/cl/1209259,
which got reverted in https://cuelang.org/cl/1209359 to un-break Unity
while we figured out why we couldn't get it to work as expected.

Currently if a schema has a doc comment, that comment is lost when it's
passed to DefineSchema, so add an argument that lets the callback see
it. This is a breaking API change, but this API is very deliberately
marked as experimental so that shouldn't matter.

An alternative that would not involve breaking the API might be to add
the comment to the Expr, but that proves quite awkward to use because in
practice callers tend to be defining a field at a path so they'd have to
jump through the hoop of extracting (and removing) the comment from the
expression only to add it back into the outer struct literal.

Signed-off-by: Roger Peppe <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
Change-Id: I45564d3f0f7ba34f435a8adb47f341dc71f187ac
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1209367

Author: rogpeppe

encoding/jsonschema/decode.go

@@ -101,6 +101,9 @@ type definedSchema struct {
 	// schema holds the actual syntax for the schema. This
 	// is nil if the entry was created by a reference only.
 	schema ast.Expr
+
+	// comment holds any doc comment associated with the above schema.
+	comment *ast.CommentGroup
 }
 
 // addImport registers
@@ -215,7 +218,7 @@ func (d *decoder) decode(v cue.Value) *ast.File {
 		// have been mapped to an external location.
 		for _, def := range d.defs {
 			if def.schema != nil && def.importPath != "" {
-				d.cfg.DefineSchema(def.importPath, def.path, def.schema)
+				d.cfg.DefineSchema(def.importPath, def.path, def.schema, def.comment)
 			}
 		}
 	}
@@ -749,7 +752,7 @@ func (s *state) schema(n cue.Value) ast.Expr {
 // schemaState returns a new state value derived from s.
 // n holds the JSONSchema node to translate to a schema.
 // types holds the set of possible types that the value can hold.
-func (s0 *state) schemaState(n cue.Value, types cue.Kind) (expr ast.Expr, ingo schemaInfo) {
+func (s0 *state) schemaState(n cue.Value, types cue.Kind) (expr ast.Expr, info schemaInfo) {
 	s := &state{
 		up: s0,
 		schemaInfo: schemaInfo{
@@ -763,7 +766,7 @@ func (s0 *state) schemaState(n cue.Value, types cue.Kind) (expr ast.Expr, ingo s
 	}
 	defer func() {
 		// Perhaps replace the schema expression with a reference.
-		expr = s.maybeDefine(expr)
+		expr = s.maybeDefine(expr, info)
 	}()
 	if n.Kind() == cue.BoolKind {
 		if vfrom(VersionDraft6).contains(s.schemaVersion) {
@@ -847,12 +850,13 @@ func (s0 *state) schemaState(n cue.Value, types cue.Kind) (expr ast.Expr, ingo s
 // it just returns expr itself.
 // TODO also report whether the schema has been defined at a place
 // where it can be unified with something else?
-func (s *state) maybeDefine(expr ast.Expr) ast.Expr {
+func (s *state) maybeDefine(expr ast.Expr, info schemaInfo) ast.Expr {
 	def := s.definedSchemaForNode(s.pos)
 	if def == nil || len(def.path.Selectors()) == 0 {
 		return expr
 	}
 	def.schema = expr
+	def.comment = info.comment()
 	if def.importPath == "" {
 		// It's a local definition that's not at the root.
 		if !s.builder.put(def.path, expr, s.comment()) {

encoding/jsonschema/decode_test.go

@@ -337,7 +337,11 @@ func TestMapRefExternalRefForInternalSchema(t *testing.T) {
 	v := cuecontext.New().CompileString(`
 type: "object"
 $id: "https://this.test"
-$defs: foo: type: ["number", "string"]
+$defs: foo: {
+	description: "foo can be a number or a string"
+	type: ["number", "string"]
+}
+$defs: bar: type: "boolean"
 $ref: "#/$defs/foo"
 `)
 	var calls []string
@@ -347,14 +351,19 @@ $ref: "#/$defs/foo"
 			calls = append(calls, loc.String())
 			switch loc.ID.String() {
 			case "https://this.test#/$defs/foo":
-				return "otherpkg.example/foo", cue.ParsePath("#foo"), nil
+				return "otherpkg.example/foo", cue.ParsePath("#x"), nil
+			case "https://this.test#/$defs/bar":
+				return "otherpkg.example/bar", cue.ParsePath("#x"), nil
 			case "https://this.test":
 				return "", cue.Path{}, nil
 			}
 			t.Errorf("unexpected ID")
 			return "", cue.Path{}, fmt.Errorf("unexpected ID %q passed to MapRef", loc.ID)
 		},
-		DefineSchema: func(importPath string, path cue.Path, e ast.Expr) {
+		DefineSchema: func(importPath string, path cue.Path, e ast.Expr, c *ast.CommentGroup) {
+			if c != nil {
+				ast.AddComment(e, c)
+			}
 			data, err := format.Node(e)
 			if err != nil {
 				t.Errorf("cannot format: %v", err)
@@ -368,20 +377,22 @@ $ref: "#/$defs/foo"
 	if err != nil {
 		t.Fatal(errors.Details(err, nil))
 	}
-	qt.Assert(t, qt.DeepEquals(calls, []string{
+	qt.Check(t, qt.DeepEquals(calls, []string{
 		"id=https://this.test#/$defs/foo localPath=$defs.foo",
+		"id=https://this.test#/$defs/bar localPath=$defs.bar",
 		"id=https://this.test localPath=",
 	}))
-	qt.Assert(t, qt.Equals(string(b), `
+	qt.Check(t, qt.Equals(string(b), `
 import "otherpkg.example/foo"
 
 @jsonschema(id="https://this.test")
-foo.#foo & {
+foo.#x & {
 	...
 }
 `[1:]))
-	qt.Assert(t, qt.DeepEquals(defines, map[string]string{
-		"otherpkg.example/foo.#foo": "number | string",
+	qt.Check(t, qt.DeepEquals(defines, map[string]string{
+		"otherpkg.example/bar.#x": "bool",
+		"otherpkg.example/foo.#x": "// foo can be a number or a string\nnumber | string",
 	}))
 }
 

encoding/jsonschema/jsonschema.go

@@ -216,15 +216,15 @@ type Config struct {
 	// DefineSchema is called, if not nil, for any schema that is defined
 	// within the json schema being converted but is mapped somewhere
 	// external via [Config.MapRef]. The invoker of [Extract] is
-	// responsible for defining the schema e in the correct place as described
+	// responsible for defining the schema in the correct place as described
 	// by the import path and its relative CUE path.
 	//
 	// The importPath and path are exactly as returned by [Config.MapRef].
 	// If this or [Config.MapRef] is nil this function will never be called.
 	// Note that importPath will never be empty, because if MapRef
 	// returns an empty importPath, it's specifying an internal schema
 	// which will be defined accordingly.
-	DefineSchema func(importPath string, path cue.Path, e ast.Expr)
+	DefineSchema func(importPath string, path cue.Path, e ast.Expr, docComment *ast.CommentGroup)
 
 	// TODO: configurability to make it compatible with OpenAPI, such as
 	// - locations of definitions: #/components/schemas, for instance.