comparison: schema_versions without the 1.x safeguards

Makefile

@@ -1,6 +1,6 @@
 SHELL = /bin/sh
 
-VERSION=1.9.0
+VERSION=2.0.0
 BUILD=`git rev-parse HEAD`
 
 LDFLAGS=-ldflags "-w -s \

fakeserver/server_test.go

@@ -20,8 +20,9 @@ import (
 )
 
 var testSchema = `
-serializer_version: 1
-schema_version: "2020011774023"
+serializer_version: 2
+schema_versions:
+- "2020011774023"
 splits:
 - name: test.test_experiment
   weights:
@@ -34,8 +35,8 @@ splits:
 `
 
 var otherTestSchema = `{
-  "serializer_version": 1,
-  "schema_version": "2020011774023",
+  "serializer_version": 2,
+  "schema_versions": ["2020011774023"],
   "splits": [
     {
       "name": "test.json_experiment",

migrationmanagers/migrationmanagers.go

@@ -94,8 +94,8 @@ func (m *MigrationManager) ApplyToSchema(migrationRepo migrations.Repository, id
 	}
 
 	appliedVersion := m.migration.MigrationVersion()
-	if appliedVersion != nil && m.schema.SchemaVersion < *appliedVersion {
-		m.schema.SchemaVersion = *appliedVersion
+	if appliedVersion != nil {
+		m.schema.AddVersion(*appliedVersion)
 	}
 	return nil
 }
@@ -160,10 +160,5 @@ func (m *MigrationManager) SyncVersion() error {
 		return fmt.Errorf("got %d status code", resp.StatusCode)
 	}
 
-	appliedVersion := m.migration.MigrationVersion()
-	if m.schema.SchemaVersion < *appliedVersion {
-		m.schema.SchemaVersion = *appliedVersion
-	}
-
 	return nil
 }

schema/schema.go

@@ -1,6 +1,8 @@
 package schema
 
 import (
+	"bytes"
+	"crypto/sha1"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -63,9 +65,10 @@ func Generate() (*serializers.Schema, error) {
 	return schema, nil
 }
 
-// Write a schema to disk after alpha-sorting its resources
+// Write a schema to disk after sorting its resources into a stable order
 func Write(schema *serializers.Schema) error {
 	SortAlphabetically(schema)
+	sortSchemaVersions(schema)
 
 	schemaPath, _ := findSchemaPath()
 
@@ -204,17 +207,35 @@ func applyAllMigrationsToSchema(schema *serializers.Schema) error {
 			return err
 		}
 	}
-	if len(versions) != 0 {
-		schema.SchemaVersion = versions[len(versions)-1]
-	}
+	schema.SchemaVersions = versions
 	return nil
 }
 
+// sortSchemaVersions orders the applied-version list by the SHA-1 of each
+// version. The order is otherwise meaningless; hashing scatters newly added
+// versions through the list instead of clustering them by timestamp, so two
+// branches that each append a migration rarely touch the same lines. Hashes
+// are precomputed so each version is hashed once rather than on every
+// comparison.
+func sortSchemaVersions(schema *serializers.Schema) {
+	hashes := make(map[string][sha1.Size]byte, len(schema.SchemaVersions))
+	for _, version := range schema.SchemaVersions {
+		hashes[version] = sha1.Sum([]byte(version))
+	}
+	sort.Slice(schema.SchemaVersions, func(i, j int) bool {
+		a := hashes[schema.SchemaVersions[i]]
+		b := hashes[schema.SchemaVersions[j]]
+		return bytes.Compare(a[:], b[:]) < 0
+	})
+}
+
 // SortAlphabetically sorts the schema's resource slices by their natural keys
 func SortAlphabetically(schema *serializers.Schema) {
 	sort.Slice(schema.RemoteKills, func(i, j int) bool {
-		return schema.RemoteKills[i].Split < schema.RemoteKills[j].Split &&
-			schema.RemoteKills[i].Reason < schema.RemoteKills[j].Reason
+		if schema.RemoteKills[i].Split != schema.RemoteKills[j].Split {
+			return schema.RemoteKills[i].Split < schema.RemoteKills[j].Split
+		}
+		return schema.RemoteKills[i].Reason < schema.RemoteKills[j].Reason
 	})
 	sort.Slice(schema.FeatureCompletions, func(i, j int) bool {
 		return schema.FeatureCompletions[i].FeatureGate < schema.FeatureCompletions[j].FeatureGate

schema/schema_test.go

@@ -0,0 +1,80 @@
+package schema
+
+import (
+	"os"
+	"path/filepath"
+	"strconv"
+	"testing"
+
+	"github.com/Betterment/testtrack-cli/serializers"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSortSchemaVersionsByHash(t *testing.T) {
+	schema := &serializers.Schema{SchemaVersions: []string{
+		"2020011712345",
+		"2020011712346",
+		"2020011712347",
+		"2020011712348",
+		"2020011712349",
+	}}
+	sortSchemaVersions(schema)
+
+	expected := []string{
+		"2020011712349", // 0dfad45993f8e2122515814d8e62f676fcb33841
+		"2020011712347", // 13af4b988d91f57f71abeb0d657c24a18fba27d7
+		"2020011712346", // 529bb729c31584a72d98669cf1daa0b9509379ce
+		"2020011712348", // a467c4bdcdbdac7030e77951932029e03a8d0920
+		"2020011712345", // bef98a4d8faa7aea1b347fb834a25c01be3c44d2
+	}
+	require.Equal(t, expected, schema.SchemaVersions, "versions should be ordered by sha1 of the version")
+}
+
+func TestGenerateRecordsAllMigrationVersions(t *testing.T) {
+	dir := t.TempDir()
+	t.Chdir(dir)
+	require.NoError(t, os.MkdirAll("testtrack/migrate", 0755))
+
+	versions := []string{"2020011712345", "2020011712346", "2020011712347"}
+	for i, version := range versions {
+		writeSplitMigration(t, version, []string{"alpha_experiment", "bravo_experiment", "charlie_experiment"}[i])
+	}
+
+	schema, err := Generate()
+	require.NoError(t, err)
+
+	require.ElementsMatch(t, versions, schema.SchemaVersions,
+		"every migration version should be recorded in schema_versions")
+}
+
+func writeSplitMigration(t *testing.T, version, name string) {
+	t.Helper()
+	contents := "serializer_version: " + strconv.Itoa(serializers.SerializerVersion) + "\n" +
+		"split:\n" +
+		"  name: " + name + "\n" +
+		"  weights:\n" +
+		"    control: 50\n" +
+		"    treatment: 50\n"
+	path := filepath.Join("testtrack", "migrate", version+"_"+name+".yml")
+	require.NoError(t, os.WriteFile(path, []byte(contents), 0644))
+}
+
+func TestSortAlphabeticallyRemoteKillsIsDeterministic(t *testing.T) {
+	// These three kills are mutually incomparable under the old buggy
+	// comparator (Split < Split && Reason < Reason), which made the written
+	// order depend on input order.
+	kills := []serializers.RemoteKill{
+		{Split: "a_split", Reason: "z_reason"},
+		{Split: "b_split", Reason: "m_reason"},
+		{Split: "c_split", Reason: "a_reason"},
+	}
+	forward := &serializers.Schema{RemoteKills: []serializers.RemoteKill{kills[0], kills[1], kills[2]}}
+	reversed := &serializers.Schema{RemoteKills: []serializers.RemoteKill{kills[2], kills[1], kills[0]}}
+
+	SortAlphabetically(forward)
+	SortAlphabetically(reversed)
+
+	require.Equal(t, forward.RemoteKills, reversed.RemoteKills,
+		"remote kills must sort to the same order regardless of input order")
+	require.Equal(t, []serializers.RemoteKill{kills[0], kills[1], kills[2]}, forward.RemoteKills)
+}

schemaloaders/schemaloaders.go

@@ -71,17 +71,27 @@ func (s *SchemaLoader) Load() error {
 		}
 	}
 
+	appliedVersions := make(map[string]bool, len(s.schema.SchemaVersions))
+	for _, version := range s.schema.SchemaVersions {
+		appliedVersions[version] = true
+	}
+
+	unrecorded := false
 	for _, version := range s.migrationRepo.SortedVersions() {
-		if version > s.schema.SchemaVersion {
-			fmt.Println("Schema load complete, but there are migrations newer than the schema file - run testtrack migrate to apply them.")
-			break
+		if !appliedVersions[version] {
+			unrecorded = true
+			continue
 		}
 		err := migrationmanagers.NewWithServer((*s.migrationRepo)[version], s.server).SyncVersion()
 		if err != nil {
 			return err
 		}
 	}
 
+	if unrecorded {
+		fmt.Println("Schema load complete, but there are migrations on disk not recorded in the schema file - run testtrack schema generate to update it.")
+	}
+
 	return nil
 }
 

schemaloaders/schemaloaders_test.go

@@ -0,0 +1,99 @@
+package schemaloaders
+
+import (
+	"io"
+	"net/http"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/Betterment/testtrack-cli/migrations"
+	"github.com/Betterment/testtrack-cli/serializers"
+	"github.com/Betterment/testtrack-cli/splits"
+	"github.com/stretchr/testify/require"
+)
+
+const unrecordedWarning = "migrations on disk not recorded in the schema file"
+
+// fakeServer records the paths it's asked to POST and always succeeds.
+type fakeServer struct {
+	postedPaths []string
+}
+
+func (f *fakeServer) Get(string, interface{}) error { return nil }
+
+func (f *fakeServer) Delete(string) error { return nil }
+
+func (f *fakeServer) Post(path string, _ interface{}) (*http.Response, error) {
+	f.postedPaths = append(f.postedPaths, path)
+	return &http.Response{StatusCode: 204, Body: io.NopCloser(strings.NewReader(""))}, nil
+}
+
+func TestLoadSyncsOnlyRecordedVersionsAndWarnsOnce(t *testing.T) {
+	server := &fakeServer{}
+	// Two versions are recorded in the schema; two migration files on disk are not.
+	schema := &serializers.Schema{SchemaVersions: []string{"2020011712345", "2020011712346"}}
+	repo := migrations.Repository{
+		"2020011712345": newSplitMigration(t, "2020011712345"),
+		"2020011712346": newSplitMigration(t, "2020011712346"),
+		"2020011712347": newSplitMigration(t, "2020011712347"),
+		"2020011712348": newSplitMigration(t, "2020011712348"),
+	}
+	loader := &SchemaLoader{server: server, schema: schema, migrationRepo: &repo}
+
+	out := captureStdout(t, func() {
+		require.NoError(t, loader.Load())
+	})
+
+	require.Equal(t, 1, strings.Count(out, unrecordedWarning),
+		"warning should print exactly once no matter how many unrecorded migrations exist")
+	// Only the two recorded versions get marked applied on the server; the
+	// schema here has no splits/etc., so these are the only posts.
+	require.Equal(t, []string{"api/v2/migrations", "api/v2/migrations"}, server.postedPaths)
+}
+
+func TestLoadDoesNotWarnWhenEveryMigrationIsRecorded(t *testing.T) {
+	server := &fakeServer{}
+	schema := &serializers.Schema{SchemaVersions: []string{"2020011712345", "2020011712346"}}
+	repo := migrations.Repository{
+		"2020011712345": newSplitMigration(t, "2020011712345"),
+		"2020011712346": newSplitMigration(t, "2020011712346"),
+	}
+	loader := &SchemaLoader{server: server, schema: schema, migrationRepo: &repo}
+
+	out := captureStdout(t, func() {
+		require.NoError(t, loader.Load())
+	})
+
+	require.NotContains(t, out, unrecordedWarning)
+	require.Equal(t, []string{"api/v2/migrations", "api/v2/migrations"}, server.postedPaths)
+}
+
+func newSplitMigration(t *testing.T, version string) migrations.IMigration {
+	t.Helper()
+	migration, err := splits.FromFile(&version, &serializers.SplitYAML{
+		Name:    "split_" + version,
+		Weights: map[string]int{"control": 50, "treatment": 50},
+	})
+	require.NoError(t, err)
+	return migration
+}
+
+func captureStdout(t *testing.T, fn func()) string {
+	t.Helper()
+	original := os.Stdout
+	r, w, err := os.Pipe()
+	require.NoError(t, err)
+	os.Stdout = w
+	defer func() {
+		os.Stdout = original
+		r.Close()
+	}()
+
+	fn()
+	require.NoError(t, w.Close())
+
+	out, err := io.ReadAll(r)
+	require.NoError(t, err)
+	return string(out)
+}

serializers/serializers.go

@@ -4,8 +4,15 @@ import (
 	"gopkg.in/yaml.v2"
 )
 
-// SerializerVersion is the current version of the migration file format so we can evolve over time
-const SerializerVersion = 1
+// SerializerVersion is the current version of the file formats so we can
+// evolve them over time.
+//
+// Version 2 replaced the single scalar schema_version high-water-mark with a
+// schema_versions list of every applied migration version, eliminating the
+// merge-conflict hotspot that the scalar created (every new migration rewrote
+// the same line). The list is ordered by a hash of each version so concurrently
+// added migrations scatter through the file instead of clustering.
+const SerializerVersion = 2
 
 // MigrationVersion is a JSON-marshalable representation of migration version (timestamp)
 type MigrationVersion struct {
@@ -90,13 +97,24 @@ type SchemaSplit struct {
 // migration validation and bootstrapping of new ecosystems
 type Schema struct {
 	SerializerVersion  int                 `yaml:"serializer_version" json:"serializer_version"`
-	SchemaVersion      string              `yaml:"schema_version" json:"schema_version"`
+	SchemaVersions     []string            `yaml:"schema_versions,omitempty" json:"schema_versions,omitempty"`
 	Splits             []SchemaSplit       `yaml:"splits,omitempty" json:"splits,omitempty"`
 	IdentifierTypes    []IdentifierType    `yaml:"identifier_types,omitempty" json:"identifier_types,omitempty"`
 	RemoteKills        []RemoteKill        `yaml:"remote_kills,omitempty" json:"remote_kills,omitempty"`
 	FeatureCompletions []FeatureCompletion `yaml:"feature_completions,omitempty" json:"feature_completions,omitempty"`
 }
 
+// AddVersion records a migration version as applied in the schema, ignoring
+// duplicates. Ordering is handled at write time, so callers needn't sort.
+func (s *Schema) AddVersion(version string) {
+	for _, v := range s.SchemaVersions {
+		if v == version {
+			return
+		}
+	}
+	s.SchemaVersions = append(s.SchemaVersions, version)
+}
+
 // LegacySchema represents the Rails migration-piggybacked testtrack schema files of old
 type LegacySchema struct {
 	IdentifierTypes []string      `yaml:"identifier_types"`