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

thema: Initial pass at Go lenses/migrations #187

Merged
merged 8 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 114 additions & 2 deletions bind.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package thema

import (
"bytes"
"fmt"

"cuelang.org/go/cue"
cerrors "cuelang.org/go/cue/errors"
"cuelang.org/go/cue/token"
"github.com/cockroachdb/errors"

terrors "github.com/grafana/thema/errors"
"github.com/grafana/thema/internal/compat"
)
Expand Down Expand Up @@ -39,10 +41,26 @@ type maybeLineage struct {

allv []SyntacticVersion

implens []ImperativeLens

// translation plan, tracks whether each lens is handled in cue or go
transplan map[lensID]bool

// The raw input value is the root of a package instance
// rawIsPackage bool
}

// to, from
type lensID [2]SyntacticVersion
sdboyer marked this conversation as resolved.
Show resolved Hide resolved

func lid(to, from SyntacticVersion) lensID {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, reason for not defining this as from, to? This seems backward, I would say "I am going from a to b", not "I am going to b from a".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a good one, no. it's all a weird artifact of probably mistakenly trying to be consistent with the expected sort ordering in lenses, where we sort first by to and then by from. (That declared sort ordering requirement only applies for CUE lenses, not for these Go things)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for curiosity; Could you give more context about the reasons behind such requirement, please? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring the inputs to be manually sorted by the user was the main way we got away from the need to dynamically sort them in CUE - which was the main source of our performance issues fixed in #173

Copy link
Contributor

@joanlopez joanlopez Jul 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant specifically about the order: first by to and then by from

PS: I guess it's because the heuristics to use that sorted list of lenses is easier this way (first by to), but was asking just for curiosity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh - i'm not attached to that as an ordering, either. My motivation was just that, assuming an ascending version sort order (same as with schemas), it seemed preferable to have forward lenses precede reverse ones in the list.

return lensID{to, from}
}

func (id lensID) String() string {
return fmt.Sprintf("%d -> %d", id[1], id[0])
}

func (ml *maybeLineage) checkGoValidity(cfg *bindConfig) error {
schiter, err := ml.uni.LookupPath(cue.MakePath(cue.Str("schemas"))).List()
if err != nil {
Expand Down Expand Up @@ -167,6 +185,12 @@ func (ml *maybeLineage) checkNativeValidity(cfg *bindConfig) error {
}

func (ml *maybeLineage) checkLensesOrder() error {
// Two distinct validation paths, depending on whether the lenses were defined in
// Go or CUE.
if len(ml.implens) > 0 {
return ml.checkGoLensCompleteness()
}

lensIter, err := ml.uni.LookupPath(cue.MakePath(cue.Str("lenses"))).List()
if err != nil {
return nil // no lenses found
Expand All @@ -179,7 +203,7 @@ func (ml *maybeLineage) checkLensesOrder() error {
return err
}

if err := checkLensesOrder(previous, &curr); err != nil {
if err := ml.doCheck(previous, &curr, ml.transplan); err != nil {
sdboyer marked this conversation as resolved.
Show resolved Hide resolved
return err
}

Expand All @@ -189,6 +213,86 @@ func (ml *maybeLineage) checkLensesOrder() error {
return nil
}

func (ml *maybeLineage) checkGoLensCompleteness() error {
all := make(map[lensID]bool)
for _, lens := range ml.implens {
all[lid(lens.To, lens.From)] = true
}

var missing []lensID

var prior SyntacticVersion
for _, sch := range ml.schlist[1:] {
// there must always at least be a reverse lens
v := sch.Version()
revid := lid(prior, v)

if !all[revid] {
missing = append(missing, revid)
} else {
delete(all, revid)
}

if v[0] != prior[0] {
// if we crossed a major version, there must also be a forward lens
fwdid := lid(v, prior)
if !all[fwdid] {
missing = append(missing, fwdid)
} else {
delete(all, fwdid)
}
}
prior = v
}

// TODO is it worth making each sub-item into its own error type?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don't see any need (at least for now), to explicitly model every item into its own error, as far as we bring that information back to the consumer.

if len(missing) > 0 {
b := new(bytes.Buffer)

fmt.Fprintf(b, "Go migrations not provided for the following version pairs:\n")
for _, mlid := range missing {
fmt.Fprint(b, "\t", mlid, "\n")
}
return errors.Mark(errors.New(b.String()), terrors.ErrMissingLenses)
}

if len(all) > 0 {
b := new(bytes.Buffer)

fmt.Fprintf(b, "Go migrations erroneously provided for the following version pairs:\n")
// walk the slice so output is reliably ordered
for _, lens := range ml.implens {
// if it's not in the list it's because it was expected & already processed
elid := lid(lens.To, lens.From)
if _, has := all[elid]; !has {
continue
}
if !synvExists(ml.allv, lens.To) {
fmt.Fprintf(b, "\t%s (schema version %s does not exist", elid, lens.To)
} else if !synvExists(ml.allv, lens.From) {
fmt.Fprintf(b, "\t%s (schema version %s does not exist", elid, lens.From)
} else if elid[0] == elid[1] {
fmt.Fprintf(b, "\t%s (self-migrations not allowed)", elid)
} else if elid[0].Less(elid[1]) {
sdboyer marked this conversation as resolved.
Show resolved Hide resolved
// reverse lenses
// only possibility is non-sequential versions connected
fmt.Fprintf(b, "\t%s (%s is predecessor of %s, not %s)", elid, ml.allv[searchSynv(ml.allv, elid[1])-1], elid[1], elid[0])
} else {
// forward lenses
// either a minor lens was provided, or non-sequential versions connected
if lens.To[0] != lens.From[0] {
fmt.Fprintf(b, "\t%s (minor version upgrades are handled automatically)", elid)
} else {
fmt.Fprintf(b, "\t%s (%s is successor of %s, not %s)", elid, ml.allv[searchSynv(ml.allv, elid[1])+1], elid[1], elid[0])
}
}
}
return errors.Mark(errors.New(b.String()), terrors.ErrErroneousLenses)
}

return nil
}

type lensVersionDef struct {
to SyntacticVersion
from SyntacticVersion
Expand All @@ -209,7 +313,7 @@ func newLensVersionDef(val cue.Value) (lensVersionDef, error) {
return lensVersionDef{to: to, from: from}, err
}

func checkLensesOrder(prev, curr *lensVersionDef) error {
func (ml *maybeLineage) doCheck(prev, curr *lensVersionDef, gomigs map[lensID]bool) error {
if prev == nil {
return nil
}
Expand All @@ -218,6 +322,14 @@ func checkLensesOrder(prev, curr *lensVersionDef) error {
return nil
}

// This check will become more useful if/when we allow a mix of lenses written in CUE and Go.
id := lid(curr.to, curr.from)
if gomigs[id] {
return errors.Mark(
errors.Errorf("lens version [to: %s, from: %s] was also provided as a Go migration", curr.to, curr.from),
terrors.ErrDuplicateLenses)
}

if curr.to.Less(prev.to) {
return errors.Mark(
errors.Errorf("lens version [to: %s, from: %s] is not greater than previous lens version [to: %s, from: %s]", curr.to, curr.from, prev.to, prev.from),
Expand Down
14 changes: 14 additions & 0 deletions errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,20 @@ var (
// ErrInvalidLensesOrder indicates that lenses are in the wrong order - they must be sorted by `to`, then `from`.
ErrInvalidLensesOrder = errors.New("lenses in lineage are not ordered by version")

// ErrDuplicateLenses indicates that a lens was defined declaratively in CUE, but the same lens
// was also provided as a Go function to BindLineage.
ErrDuplicateLenses = errors.New("lens is declared in both CUE and Go")

// ErrMissingLenses indicates that the lenses provided to BindLineage in either
// CUE or Go were missing at least one of the expected lenses determined by the
// set of schemas in the lineage.
ErrMissingLenses = errors.New("not all expected lenses were provided")

// ErrErroneousLenses indicates that a lens was provided to BindLineage in either
// CUE or Go that was not one of the expected lenses determined by the set of
// schemas in the lineage.
ErrErroneousLenses = errors.New("unexpected lenses were erroneously provided")

// ErrVersionNotExist indicates that no schema exists in a lineage with a
// given version.
ErrVersionNotExist = errors.New("lineage does not contain schema with version") // ErrNoSchemaWithVersion
Expand Down
Loading
Loading