Merge pull request #178 from grafana/sdboyer/translate-errors

feat: Return sane errors from Translate

cmd/thema/data.go

@@ -10,9 +10,10 @@ import (
 	"path/filepath"
 
 	"cuelang.org/go/cue"
+	"github.com/spf13/cobra"
+
 	"github.com/grafana/thema"
 	"github.com/grafana/thema/vmux"
-	"github.com/spf13/cobra"
 )
 
 type dataCommand struct {
@@ -180,8 +181,11 @@ func (dc *dataCommand) runTranslate(cmd *cobra.Command, args []string) error {
 	}
 
 	// Prior validations checked that the schema version exists in the lineage
-	tinst, lac := inst.Translate(dc.lla.dl.sch.Version())
-	if err := dc.validateTranslationResult(tinst, lac); err != nil {
+	tinst, lac, err := inst.Translate(dc.lla.dl.sch.Version())
+	if err != nil {
+		return err
+	}
+	if err = dc.validateTranslationResult(tinst, lac); err != nil {
 		return err
 	}
 

errors/errors.go

@@ -35,17 +35,17 @@ type ValidationError struct {
 
 // Unwrap implements standard Go error unwrapping, relied on by errors.Is.
 //
-// All ValidationErrors wrap the general ErrNotAnInstance sentinel error.
+// All ValidationErrors wrap the general ErrInvalidData sentinel error.
 func (ve *ValidationError) Unwrap() error {
-	return ErrNotAnInstance
+	return ErrInvalidData
 }
 
 // Validation error codes/types
 var (
-	// ErrNotAnInstance is the general error that indicates some data failed validation
+	// ErrInvalidData is the general error that indicates some data failed validation
 	// against a Thema schema. Use it with errors.Is() to differentiate validation errors
 	// from other classes of failure.
-	ErrNotAnInstance = errors.New("data not a valid instance of schema")
+	ErrInvalidData = errors.New("data not a valid instance of schema")
 
 	// ErrInvalidExcessField indicates a validation failure in which the schema is
 	// treated as closed, and the data contains a field not specified in the schema.
@@ -66,6 +66,27 @@ var (
 	ErrInvalidOutOfBounds = errors.New("data is out of schema bounds")
 )
 
+// Translation errors. These all occur as a result of an invalid lens. Currently
+// these may be returned from [thema.Instance.Translate]. Eventually, it is
+// hoped that they will be caught statically in [thema.BindLineage] and cannot
+// occur at runtime.
+var (
+	// ErrInvalidLens indicates that a lens is not correctly written. It is the parent
+	// to all other lens and translation errors, and is a child of ErrInvalidLineage.
+	ErrInvalidLens = errors.New("lens is invalid")
+
+	// ErrLensIncomplete indicates that translating some valid data through
+	// a lens produced a non-concrete result. This always indicates a problem with the
+	// lens as it is written, and as such is a child of ErrInvalidLens.
+	ErrLensIncomplete = errors.New("result of lens translation is not concrete")
+
+	// ErrLensResultIsInvalidData indicates that translating some valid data through a
+	// lens produced a result that was not an instance of the target schema. This
+	// always indicates a problem with the lens as it is written, and as such is a
+	// child of ErrInvalidLens.
+	ErrLensResultIsInvalidData = errors.New("result of lens translation is not valid for target schema")
+)
+
 // Lower level general errors
 var (
 	// ErrValueNotExist indicates that a necessary CUE value did not exist.

instance.go

@@ -4,7 +4,11 @@ import (
 	"fmt"
 
 	"cuelang.org/go/cue"
-	"cuelang.org/go/cue/errors"
+	cerrors "cuelang.org/go/cue/errors"
+	"cuelang.org/go/pkg/encoding/json"
+	"github.com/cockroachdb/errors"
+
+	terrors "github.com/grafana/thema/errors"
 )
 
 // BindInstanceType produces a TypedInstance, given an Instance and a
@@ -94,14 +98,14 @@ func (i *Instance) Dehydrate() *Instance {
 
 // AsSuccessor translates the instance into the form specified by the successor
 // schema.
-func (i *Instance) AsSuccessor() (*Instance, TranslationLacunas) {
+func (i *Instance) AsSuccessor() (*Instance, TranslationLacunas, error) {
 	i.check()
 	return i.Translate(i.sch.Successor().Version())
 }
 
 // AsPredecessor translates the instance into the form specified by the predecessor
 // schema.
-func (i *Instance) AsPredecessor() (*Instance, TranslationLacunas) {
+func (i *Instance) AsPredecessor() (*Instance, TranslationLacunas, error) {
 	i.check()
 	return i.Translate(i.sch.Predecessor().Version())
 }
@@ -187,7 +191,11 @@ func (inst *TypedInstance[T]) ValueP() T {
 // result in the exact original data. Input state preservation can be fully
 // achieved in the program depending on Thema, so we avoid introducing
 // complexity into Thema that is not essential for all use cases.
-func (i *Instance) Translate(to SyntacticVersion) (*Instance, TranslationLacunas) {
+//
+// Errors only occur in cases where lenses were written in an unexpected way -
+// for example, not all fields were mapped over, and the resulting object is not
+// concrete. All errors returned from this func will children of [terrors.ErrInvalidLens].
+func (i *Instance) Translate(to SyntacticVersion) (*Instance, TranslationLacunas, error) {
 	i.check()
 
 	// TODO define this in terms of AsSuccessor and AsPredecessor, rather than those in terms of this.
@@ -208,22 +216,28 @@ func (i *Instance) Translate(to SyntacticVersion) (*Instance, TranslationLacunas
 	}
 
 	if out.Err() != nil {
-		panic(errors.Details(out.Err(), nil))
+		return nil, nil, errors.Mark(out.Err(), terrors.ErrInvalidLens)
 	}
 
 	lac := make(multiTranslationLacunas, 0)
 	out.LookupPath(cue.MakePath(cue.Str("lacunas"))).Decode(&lac)
 
-	// Attempt to evaluate #Translate result into a concrete cue.Value, if possible.
+	// Attempt to evaluate #Translate result to remove intermediate structures created by #Translate.
 	// Otherwise, all the #Translate results are non-concrete, which leads to undesired effects.
 	raw, _ := out.LookupPath(cue.MakePath(cue.Str("result"), cue.Str("result"))).Default()
 
-	return &Instance{
-		valid: true,
-		raw:   raw,
-		name:  i.name,
-		sch:   newsch,
-	}, lac
+	// Check that the result is concrete by trying to marshal/export it as JSON
+	_, err = json.Marshal(raw)
+	if err != nil {
+		return nil, nil, errors.Mark(fmt.Errorf("lens produced a non-concrete result: %s", cerrors.Details(err, nil)), terrors.ErrLensIncomplete)
+	}
+
+	// Ensure the result is a valid instance of the target schema
+	inst, err := newsch.Validate(raw)
+	if err != nil {
+		return nil, nil, errors.Mark(err, terrors.ErrLensResultIsInvalidData)
+	}
+	return inst, lac, err
 }
 
 type multiTranslationLacunas []struct {
@@ -239,7 +253,3 @@ func (lac multiTranslationLacunas) AsList() []Lacuna {
 	}
 	return l
 }
-
-// func TranslateComposed(lin ComposedLineage) {
-
-// }

instance_test.go

@@ -35,7 +35,8 @@ func TestInstance_Translate(t *testing.T) {
 		for from := lin.First(); from != nil; from = from.Successor() {
 			for _, example := range from.Examples() {
 				for to := lin.First(); to != nil; to = to.Successor() {
-					tinst, lacunas := example.Translate(to.Version())
+					tinst, lacunas, err := example.Translate(to.Version())
+					require.NoError(t, err)
 					require.NotNil(t, tinst)
 
 					result := tinst.Underlying()
@@ -127,7 +128,8 @@ schemas: [
 	require.Equal(t, expected, got)
 
 	// Translate cue.Value (no lacunas)
-	tinst, _ := inst.Translate(SV(0, 1))
+	tinst, _, err := inst.Translate(SV(0, 1))
+	require.NoError(t, err)
 	require.Equal(t, SV(0, 0), inst.Schema().Version())
 
 	got, err = tinst.Underlying().LookupPath(cue.ParsePath("title")).String()

validate.go

@@ -50,7 +50,7 @@ func (e *onesidederr) Error() string {
 }
 
 func (e *onesidederr) Unwrap() error {
-	return terrors.ErrNotAnInstance
+	return terrors.ErrInvalidData
 }
 
 type twosidederr struct {
@@ -76,7 +76,7 @@ func (e *twosidederr) Error() string {
 }
 
 func (e *twosidederr) Unwrap() error {
-	return terrors.ErrNotAnInstance
+	return terrors.ErrInvalidData
 }
 
 // TODO differentiate this once we have generic composition to support trimming out irrelevant disj branches
@@ -87,13 +87,13 @@ type emptydisjunction struct {
 }
 
 func (e *emptydisjunction) Unwrap() error {
-	return terrors.ErrNotAnInstance
+	return terrors.ErrInvalidData
 }
 
 type validationFailure []error
 
 func (vf validationFailure) Unwrap() error {
-	return terrors.ErrNotAnInstance
+	return terrors.ErrInvalidData
 }
 
 func (vf validationFailure) Error() string {

vmux/typed.go

@@ -48,7 +48,12 @@ func NewTypedMux[T thema.Assignee](sch thema.TypedSchema[T], dec Decoder) TypedM
 			}
 
 			if inst, ierr := isch.Validate(v); ierr == nil {
-				trinst, lac := inst.Translate(sch.Version())
+				trinst, lac, err := inst.Translate(sch.Version())
+				if err != nil {
+					return nil, nil, err
+				}
+
+				// TODO perf: introduce a typed translator to avoid wastefully re-binding the go type every time
 				tinst, err := thema.BindInstanceType(trinst, sch)
 				if err != nil {
 					panic(fmt.Errorf("unreachable, instance type should always be bindable: %w", err))

vmux/untyped.go

@@ -48,8 +48,7 @@ func NewUntypedMux(sch thema.Schema, dec Decoder) UntypedMux {
 			}
 
 			if inst, ierr := isch.Validate(v); ierr == nil {
-				trinst, lac := inst.Translate(sch.Version())
-				return trinst, lac, nil
+				return inst.Translate(sch.Version())
 			}
 		}