From 3bbe151923366583580861b6ee850590169a6f74 Mon Sep 17 00:00:00 2001 From: Badr Ezzir Date: Tue, 4 Aug 2026 00:11:06 +0100 Subject: [PATCH] ENG-00000: Make generated code and paths.yaml deterministic Generated artifacts differed between machines and between runs, so regenerating produced large spurious diffs. Fix each source of non-determinism where the unordered data enters the pipeline. - codegen: run emitted code through go/format and correct the EmbedFS snippet indentation. On a format error the unformatted result is kept. - column discovery: sort information_schema columns by ordinal position. Those queries carry no ORDER BY, so driver ordering leaked into generated struct field order. Columns inferred from a result set carry no position and are left untouched, preserving the projection order the query already established. - route listing: sort paths.yaml entries by URL. The recursive listing reflected directory enumeration order, which varies by filesystem and shifts whenever route files are rewritten, so partial regeneration reshuffled thousands of lines. Route lookup is order independent: the matcher builds a trie and prefers exact matches. Adds unit tests covering each sort and the gofmt stability of generated output. Co-Authored-By: Claude Opus 5 (1M context) --- repository/codegen.go | 11 +++- repository/codegen_embedfs_test.go | 96 ++++++++++++++++++++++++++++++ repository/path/service.go | 14 +++++ repository/path/sort_test.go | 94 +++++++++++++++++++++++++++++ view/column/discover.go | 19 ++++++ view/column/discover_sort_test.go | 83 ++++++++++++++++++++++++++ 6 files changed, 314 insertions(+), 3 deletions(-) create mode 100644 repository/codegen_embedfs_test.go create mode 100644 repository/path/sort_test.go create mode 100644 view/column/discover_sort_test.go diff --git a/repository/codegen.go b/repository/codegen.go index 137ac8c3..11b9135b 100644 --- a/repository/codegen.go +++ b/repository/codegen.go @@ -13,6 +13,7 @@ import ( "github.com/viant/tagly/format/text" "github.com/viant/toolbox/data" "github.com/viant/xreflect" + "go/format" "path" "reflect" "strconv" @@ -190,14 +191,18 @@ func (c *Component) GenerateOutputCode(ctx context.Context, withDefineComponent, if withEmbed { embedderCode := fmt.Sprintf(` - func (i *%vInput) EmbedFS() *embed.FS { - return &%vFS - }`, componentName, componentName) +func (i *%vInput) EmbedFS() *embed.FS { + return &%vFS +} +`, componentName, componentName) builder.WriteString(embedderCode) } result := builder.String() result = c.View.Resource().ReverseSubstitutes(result) + if formatted, err := format.Source([]byte(result)); err == nil { + result = string(formatted) + } return result } diff --git a/repository/codegen_embedfs_test.go b/repository/codegen_embedfs_test.go new file mode 100644 index 00000000..ccd5efc7 --- /dev/null +++ b/repository/codegen_embedfs_test.go @@ -0,0 +1,96 @@ +package repository + +import ( + "context" + "go/format" + "reflect" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/viant/datly/repository/contract" + "github.com/viant/datly/view" + "github.com/viant/datly/view/state" + "github.com/viant/xreflect" +) + +func newEmbedFSTestComponent(t *testing.T) *Component { + t.Helper() + + resource := view.EmptyResource() + rootView := view.NewView("active_advertiser", "ACTIVE_ADVERTISER") + rootView.Connector = &view.Connector{Connection: view.Connection{DBConfig: view.DBConfig{Name: "ci_ads"}}} + rootView.Template = &view.Template{Source: "SELECT ID FROM ACTIVE_ADVERTISER"} + rootView.Schema = state.NewSchema(reflect.TypeOf([]*struct { + Id *int `sqlx:"ID"` + }{})) + resource.Types = []*view.TypeDefinition{ + {Name: "ActiveAdvertiserView", Package: "universalpixel", DataType: `struct{Id *int ` + "`sqlx:\"ID\"`" + `;}`}, + } + require.NoError(t, resource.TypeRegistry().Register("ActiveAdvertiserView", + xreflect.WithPackage("universalpixel"), + xreflect.WithReflectType(reflect.TypeOf(struct { + Id *int `sqlx:"ID"` + }{})))) + rootView.SetResource(resource) + resource.AddViews(rootView) + + inputType, err := state.NewType(state.WithParameters(state.Parameters{ + &state.Parameter{Name: "AdvertiserId", In: state.NewQueryLocation("advertiserId"), Schema: state.NewSchema(reflect.TypeOf(0))}, + }), state.WithResource(&reportTestResource{})) + require.NoError(t, err) + inputType.Name = "ActiveAdvertiserInput" + inputType.Package = "universalpixel" + + outputType, err := state.NewType(state.WithParameters(state.Parameters{ + &state.Parameter{Name: "Data", In: state.NewOutputLocation("view"), Schema: &state.Schema{Name: "ActiveAdvertiserView", Package: "universalpixel", Cardinality: state.Many}}, + }), state.WithResource(rootView.Resource())) + require.NoError(t, err) + outputType.Name = "ActiveAdvertiserOutput" + outputType.Package = "universalpixel" + + return &Component{ + Path: contract.Path{Method: "GET", URI: "/v1/api/platform/universalpixel/activeadvertiser"}, + Meta: contract.Meta{Name: "ActiveAdvertiser"}, + View: rootView, + Contract: contract.Contract{ + Input: contract.Input{Type: *inputType}, + Output: contract.Output{Type: *outputType}, + }, + } +} + +// The generated EmbedFS accessor used to be emitted from a raw string literal +// indented by one tab and without a trailing newline, so every generated reader +// was unformatted Go. Consumers that ran gofmt saw the file churn back on the +// next `datly gen`. +func TestGenerateOutputCode_EmbedFSAccessorIsGoFormatted(t *testing.T) { + component := newEmbedFSTestComponent(t) + + code := component.GenerateOutputCode(context.Background(), false, true, map[string]string{}) + + assert.Contains(t, code, "\nfunc (i *ActiveAdvertiserInput) EmbedFS() *embed.FS {\n\treturn &ActiveAdvertiserFS\n}\n", + "EmbedFS accessor must be emitted at column 0 with a tab-indented body") + assert.NotContains(t, code, "\n\tfunc (", "no top-level func may be indented") + assert.True(t, strings.HasSuffix(code, "\n"), "generated file must end with a newline") +} + +func TestGenerateOutputCode_IsGofmtStable(t *testing.T) { + component := newEmbedFSTestComponent(t) + + code := component.GenerateOutputCode(context.Background(), false, true, map[string]string{}) + + formatted, err := format.Source([]byte(code)) + require.NoError(t, err, "generated code must parse") + assert.Equal(t, string(formatted), code, "generated code must already be gofmt-clean") +} + +// Regenerating an unchanged component must not produce a different file, or +// consumers get spurious diffs on every codegen run. +func TestGenerateOutputCode_IsDeterministic(t *testing.T) { + first := newEmbedFSTestComponent(t).GenerateOutputCode(context.Background(), false, true, map[string]string{}) + second := newEmbedFSTestComponent(t).GenerateOutputCode(context.Background(), false, true, map[string]string{}) + + assert.Equal(t, first, second) +} diff --git a/repository/path/service.go b/repository/path/service.go index 44118dbb..83e735f3 100644 --- a/repository/path/service.go +++ b/repository/path/service.go @@ -13,6 +13,7 @@ import ( "github.com/viant/datly/repository/version" "gopkg.in/yaml.v3" "path" + "sort" "strings" "sync" "time" @@ -151,6 +152,7 @@ func (s *Service) createPathFiles(ctx context.Context) error { if err != nil { return err } + sortByURL(candidates) rootPath := url.Path(s.URL) for _, candidate := range candidates { if candidate.IsDir() { @@ -187,6 +189,18 @@ func (s *Service) createPathFiles(ctx context.Context) error { return nil } +// sortByURL gives paths.yaml a stable entry order. The recursive listing that +// feeds it reflects directory enumeration order, which differs between +// filesystems and shifts whenever route files are rewritten - so without this +// the same repository yields a different paths.yaml on every machine, and any +// partial regeneration reshuffles thousands of lines. Route lookup itself is +// order independent: the matcher builds a trie and prefers exact matches. +func sortByURL(candidates []storage.Object) { + sort.SliceStable(candidates, func(i, j int) bool { + return candidates[i].URL() < candidates[j].URL() + }) +} + func (s *Service) buildPaths(ctx context.Context, candidate storage.Object, rootPath string) (*Item, error) { data, err := s.fs.Download(ctx, candidate) if err != nil { diff --git a/repository/path/sort_test.go b/repository/path/sort_test.go new file mode 100644 index 00000000..03ee61cf --- /dev/null +++ b/repository/path/sort_test.go @@ -0,0 +1,94 @@ +package path + +import ( + spath "path" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/viant/afs/file" + "github.com/viant/afs/object" + "github.com/viant/afs/storage" +) + +func objectURLs(candidates []storage.Object) []string { + var result []string + for _, candidate := range candidates { + result = append(result, candidate.URL()) + } + return result +} + +func newObject(URL string) storage.Object { + name := spath.Base(URL) + return object.New(URL, file.NewInfo(name, 0, file.DefaultFileOsMode, time.Now(), false), nil) +} + +func newObjects(URLs ...string) []storage.Object { + var result []storage.Object + for _, URL := range URLs { + result = append(result, newObject(URL)) + } + return result +} + +// The recursive listing behind paths.yaml reflects directory enumeration order, +// so without an explicit sort the same repository produces a different +// paths.yaml on every machine and any partial regeneration reshuffles it. +func TestSortByURL(t *testing.T) { + testCases := []struct { + description string + urls []string + expect []string + }{ + { + description: "enumeration order is normalised to lexical order", + urls: []string{ + "file:///repo/routes/system/session/session.yaml", + "file:///repo/routes/mdp/adorder/forecast.yaml", + "file:///repo/routes/platform/agency/agency.yaml", + }, + expect: []string{ + "file:///repo/routes/mdp/adorder/forecast.yaml", + "file:///repo/routes/platform/agency/agency.yaml", + "file:///repo/routes/system/session/session.yaml", + }, + }, + { + description: "already sorted stays sorted", + urls: []string{"file:///repo/routes/a.yaml", "file:///repo/routes/b.yaml"}, + expect: []string{"file:///repo/routes/a.yaml", "file:///repo/routes/b.yaml"}, + }, + { + description: "single entry", + urls: []string{"file:///repo/routes/only.yaml"}, + expect: []string{"file:///repo/routes/only.yaml"}, + }, + { + description: "empty listing is a no-op", + urls: nil, + expect: nil, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + candidates := newObjects(testCase.urls...) + sortByURL(candidates) + assert.Equal(t, testCase.expect, objectURLs(candidates)) + }) + } +} + +// Sorting must be idempotent, otherwise repeated regeneration still churns. +func TestSortByURL_Idempotent(t *testing.T) { + candidates := newObjects( + "file:///repo/routes/z.yaml", + "file:///repo/routes/a.yaml", + "file:///repo/routes/m.yaml", + ) + sortByURL(candidates) + first := objectURLs(candidates) + sortByURL(candidates) + assert.Equal(t, first, objectURLs(candidates)) +} diff --git a/view/column/discover.go b/view/column/discover.go index 5c779fd9..7c03b4a7 100644 --- a/view/column/discover.go +++ b/view/column/discover.go @@ -15,6 +15,7 @@ import ( "github.com/viant/sqlx/io" "github.com/viant/sqlx/io/config" "reflect" + "sort" "github.com/viant/sqlx/metadata/sink" "github.com/viant/xreflect" @@ -250,9 +251,27 @@ func readSinkColumns(ctx context.Context, db *sql.DB, table string) ([]sink.Colu if len(columns) == 0 { return nil, vErr } + sortByPosition(columns) return columns, err } +// sortByPosition orders columns by their ordinal position in the table. +// The information_schema queries behind config.Columns carry no ORDER BY, so +// the driver may return columns in any order - which would otherwise leak into +// generated struct field order and produce spurious diffs between machines. +// Columns inferred from a result set carry no position; leaving them stable +// preserves the projection order the query already established. +func sortByPosition(columns []sink.Column) { + for _, column := range columns { + if column.Position == 0 { + return + } + } + sort.SliceStable(columns, func(i, j int) bool { + return columns[i].Position < columns[j].Position + }) +} + func parseQuery(SQL string) (string, string, sqlparser.Columns) { sqlQuery, _ := sqlparser.ParseQuery(SQL) var table string diff --git a/view/column/discover_sort_test.go b/view/column/discover_sort_test.go new file mode 100644 index 00000000..0e9d0798 --- /dev/null +++ b/view/column/discover_sort_test.go @@ -0,0 +1,83 @@ +package column + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/viant/sqlx/metadata/sink" +) + +func names(columns []sink.Column) []string { + var result []string + for _, column := range columns { + result = append(result, column.Name) + } + return result +} + +// config.Columns runs an information_schema query with no ORDER BY, so the +// driver may hand back columns in any order. Without a sort, that order leaks +// into generated struct field order and churns on every regeneration. +func TestSortByPosition(t *testing.T) { + testCases := []struct { + description string + columns []sink.Column + expect []string + }{ + { + description: "alphabetical metadata order is restored to table order", + columns: []sink.Column{ + {Name: "CREATED", Position: 5}, + {Name: "CREATED_USER", Position: 7}, + {Name: "FEE_DOMAIN", Position: 4}, + {Name: "FEE_TYPE_ID", Position: 3}, + {Name: "ID", Position: 1}, + {Name: "NAME", Position: 2}, + {Name: "UPDATED", Position: 6}, + {Name: "UPDATED_USER", Position: 8}, + }, + expect: []string{"ID", "NAME", "FEE_TYPE_ID", "FEE_DOMAIN", "CREATED", "UPDATED", "CREATED_USER", "UPDATED_USER"}, + }, + { + description: "already ordered stays ordered", + columns: []sink.Column{ + {Name: "ID", Position: 1}, + {Name: "NAME", Position: 2}, + }, + expect: []string{"ID", "NAME"}, + }, + { + description: "result-set inferred columns carry no position, so projection order is preserved", + columns: []sink.Column{ + {Name: "TOTAL_SPEND"}, + {Name: "AGENCY_ID"}, + }, + expect: []string{"TOTAL_SPEND", "AGENCY_ID"}, + }, + { + description: "empty input is a no-op", + columns: []sink.Column{}, + expect: nil, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + sortByPosition(testCase.columns) + assert.Equal(t, testCase.expect, names(testCase.columns)) + }) + } +} + +// Sorting must be idempotent, otherwise repeated regeneration would still churn. +func TestSortByPosition_Idempotent(t *testing.T) { + columns := []sink.Column{ + {Name: "UPDATED", Position: 6}, + {Name: "ID", Position: 1}, + {Name: "NAME", Position: 2}, + } + sortByPosition(columns) + first := names(columns) + sortByPosition(columns) + assert.Equal(t, first, names(columns)) +}