From 1fb84d5d3be0979f75303945b8154f3a2b5afde6 Mon Sep 17 00:00:00 2001 From: Martin Najemi Date: Sat, 4 Apr 2026 19:15:29 +0200 Subject: [PATCH] chore: Better lockfile dep change detection Risk: low --- CHANGELOG.md | 6 + VERSION | 2 +- internal/git/git.go | 5 - internal/lockfile/lockfile.go | 320 +++++++++++++++------------------- main.go | 23 +-- 5 files changed, 155 insertions(+), 201 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ee3caa..1b8b80b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.17.0] - 2026-04-04 + +### Changed +- **Breaking:** Lockfile dep change detection now parses old and new pnpm-lock.yaml as YAML (gopkg.in/yaml.v3) instead of diffing text lines. Compares resolved versions for direct deps per importer, and BFS-walks the snapshots section to detect transitive dep version changes — a transitive change taints the direct dep that pulled it in. + ## [0.16.7] - 2026-04-04 ### Changed @@ -237,6 +242,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Multi-stage Docker build - Automated vendor upgrade workflow +[0.17.0]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.16.7...v0.17.0 [0.16.7]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.16.6...v0.16.7 [0.16.6]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.16.5...v0.16.6 [0.16.5]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.16.4...v0.16.5 diff --git a/VERSION b/VERSION index 75aa0c2..c5523bd 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.16.7 \ No newline at end of file +0.17.0 diff --git a/internal/git/git.go b/internal/git/git.go index 29ce34f..1f486c5 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -58,11 +58,6 @@ func MergeBase(branch string) (string, error) { return base, nil } -// DiffSincePath returns the unified diff for a specific path since the given commit. -func DiffSincePath(commit string, path string) (string, error) { - return Cmd("diff", commit, "--", path) -} - // ShowFile returns the content of a file at a specific commit. // Returns empty string and no error if the file didn't exist at that commit. func ShowFile(commit string, path string) (string, error) { diff --git a/internal/lockfile/lockfile.go b/internal/lockfile/lockfile.go index d775f00..3b65618 100644 --- a/internal/lockfile/lockfile.go +++ b/internal/lockfile/lockfile.go @@ -1,242 +1,200 @@ package lockfile import ( - "os" + "fmt" "path/filepath" - "strconv" "strings" "gopkg.in/yaml.v3" ) -type depLineInfo struct { - projectFolder string - depName string +// PnpmLockfile represents a parsed pnpm-lock.yaml. +type PnpmLockfile struct { + LockfileVersion any `yaml:"lockfileVersion"` + Importers map[string]ImporterEntry `yaml:"importers"` + Snapshots map[string]SnapshotEntry `yaml:"snapshots"` } -// FindDepAffectedProjects parses a pnpm-lock.yaml and its diff to find -// which projects had direct dependency version changes. -// Returns a map of project folder → set of changed dependency package names. -// Workspace deps (version: link:...) are excluded as they're handled by the Rush dep graph. -// The subspace parameter is used to resolve importer paths (they're relative to common/temp/{subspace}/). -// TODO: handle transitive dep changes (a dep-of-a-dep changing without the direct dep version changing). -func FindDepAffectedProjects(lockfilePath string, subspace string, diffText string) map[string]map[string]bool { - if diffText == "" { +// ImporterEntry represents a project in the importers section. +type ImporterEntry struct { + Dependencies map[string]DepRef `yaml:"dependencies"` + DevDependencies map[string]DepRef `yaml:"devDependencies"` + OptionalDependencies map[string]DepRef `yaml:"optionalDependencies"` +} + +// DepRef represents a dependency reference with specifier and resolved version. +type DepRef struct { + Specifier string `yaml:"specifier"` + Version string `yaml:"version"` +} + +// SnapshotEntry represents a resolved package in the snapshots section. +type SnapshotEntry struct { + Dependencies map[string]string `yaml:"dependencies"` + OptionalDependencies map[string]string `yaml:"optionalDependencies"` +} + +// ParseLockfile parses pnpm-lock.yaml content into a structured format. +// Returns nil if content is empty or parsing fails. +func ParseLockfile(content []byte) *PnpmLockfile { + if len(content) == 0 { return nil } - - content, err := os.ReadFile(lockfilePath) - if err != nil { + var lf PnpmLockfile + if err := yaml.Unmarshal(content, &lf); err != nil { return nil } + return &lf +} - importerBase := filepath.Join("common", "temp", subspace) - lineMap, workspaceDeps := buildImporterDepLineMap(string(content), importerBase) +// Version returns the lockfileVersion as a string. +// Returns empty string if the lockfile is nil or has no version. +func (lf *PnpmLockfile) Version() string { + if lf == nil || lf.LockfileVersion == nil { + return "" + } + return fmt.Sprintf("%v", lf.LockfileVersion) +} - changedLines := parseDiffChangedLines(diffText) +// FindDepChanges compares old and new lockfiles to find which projects had +// dependency version changes (direct or transitive). +// Returns a map of project folder → set of changed direct dependency names. +// Workspace deps (version: link:...) are excluded. +// The subspace parameter resolves importer paths (relative to common/temp/{subspace}/). +func FindDepChanges(oldLf, newLf *PnpmLockfile, subspace string) map[string]map[string]bool { + if newLf == nil { + return nil + } + importerBase := filepath.Join("common", "temp", subspace) result := make(map[string]map[string]bool) - for _, line := range changedLines { - info, ok := lineMap[line] - if !ok || info.projectFolder == "" || info.depName == "" { - continue - } - if workspaceDeps[info.projectFolder] != nil && workspaceDeps[info.projectFolder][info.depName] { - continue - } - if result[info.projectFolder] == nil { - result[info.projectFolder] = make(map[string]bool) - } - result[info.projectFolder][info.depName] = true + + var oldImporters map[string]ImporterEntry + var oldSnapshots map[string]SnapshotEntry + if oldLf != nil { + oldImporters = oldLf.Importers + oldSnapshots = oldLf.Snapshots } - return result -} -// buildImporterDepLineMap reads a pnpm-lock.yaml and returns: -// - lineMap: line number (1-based) → project folder + dep name for lines in the importers section -// - workspaceDeps: project folder → dep name → true (for workspace deps identified by version: link:...) -func buildImporterDepLineMap(content string, importerBase string) (map[int]depLineInfo, map[string]map[string]bool) { - lineMap := make(map[int]depLineInfo) - workspaceDeps := make(map[string]map[string]bool) - lines := strings.Split(content, "\n") - - inImporters := false - currentImporter := "" - inDepSection := false - currentDepName := "" - - for i, line := range lines { - lineNum := i + 1 - indent := countLeadingSpaces(line) - trimmed := strings.TrimSpace(line) - - if trimmed == "" || strings.HasPrefix(trimmed, "#") { + for importerPath, newImporter := range newLf.Importers { + projectFolder := resolveImporterPath(importerPath, importerBase) + if projectFolder == "" { continue } - // Top-level section (no indent) - if indent == 0 { - if strings.HasPrefix(line, "importers:") { - inImporters = true - currentImporter = "" - inDepSection = false - currentDepName = "" - } else if inImporters { - break // Left importers section - } - continue - } + oldImporter := oldImporters[importerPath] + newDeps := mergeImporterDeps(newImporter) + oldDeps := mergeImporterDeps(oldImporter) - if !inImporters { - continue - } - - switch { - case indent == 2: - // Importer path (e.g. " ../../../sdk/libs/sdk-ui-kit:") - if !strings.HasSuffix(trimmed, ":") { + for depName, newRef := range newDeps { + if strings.HasPrefix(newRef.Version, "link:") { continue } - rawPath := strings.TrimSuffix(trimmed, ":") - rawPath = strings.Trim(rawPath, "'\"") - if rawPath == "." { - currentImporter = "" - } else { - resolved := filepath.Clean(filepath.Join(importerBase, rawPath)) - if rel, err := filepath.Rel(".", resolved); err == nil { - currentImporter = rel - } else { - currentImporter = resolved - } - } - inDepSection = false - currentDepName = "" - case indent == 4: - // Section header (dependencies, devDependencies, optionalDependencies) - if currentImporter == "" { - continue - } - if trimmed == "dependencies:" || trimmed == "devDependencies:" || trimmed == "optionalDependencies:" { - inDepSection = true - } else { - inDepSection = false - } - currentDepName = "" + oldRef := oldDeps[depName] - case indent == 6: - // Dep name (e.g. " react:" or " '@gooddata/sdk-model':") - if !inDepSection || currentImporter == "" { - continue - } - if !strings.HasSuffix(trimmed, ":") { + // Direct version change + if oldRef.Version != newRef.Version { + if result[projectFolder] == nil { + result[projectFolder] = make(map[string]bool) + } + result[projectFolder][depName] = true continue } - currentDepName = strings.TrimSuffix(trimmed, ":") - currentDepName = strings.Trim(currentDepName, "'\"") - // Map the dep name line too (in case it's added/removed in the diff) - lineMap[lineNum] = depLineInfo{ - projectFolder: currentImporter, - depName: currentDepName, - } - case indent >= 8: - // Dep property (specifier, version) - if !inDepSection || currentImporter == "" || currentDepName == "" { - continue - } - lineMap[lineNum] = depLineInfo{ - projectFolder: currentImporter, - depName: currentDepName, - } - // Detect workspace deps by version: link:... - if strings.HasPrefix(trimmed, "version:") { - version := strings.TrimSpace(strings.TrimPrefix(trimmed, "version:")) - version = strings.Trim(version, "'\"") - if strings.HasPrefix(version, "link:") { - if workspaceDeps[currentImporter] == nil { - workspaceDeps[currentImporter] = make(map[string]bool) + // Check transitive deps for changes + if len(newLf.Snapshots) > 0 { + snapshotKey := depName + "@" + newRef.Version + if hasTransitiveChanges(snapshotKey, oldSnapshots, newLf.Snapshots) { + if result[projectFolder] == nil { + result[projectFolder] = make(map[string]bool) } - workspaceDeps[currentImporter][currentDepName] = true + result[projectFolder][depName] = true } } } } - return lineMap, workspaceDeps + return result } -func countLeadingSpaces(s string) int { - for i := 0; i < len(s); i++ { - if s[i] != ' ' { - return i - } +func resolveImporterPath(importerPath, importerBase string) string { + importerPath = strings.Trim(importerPath, "'\"") + if importerPath == "." { + return "" } - return len(s) + resolved := filepath.Clean(filepath.Join(importerBase, importerPath)) + if rel, err := filepath.Rel(".", resolved); err == nil { + return rel + } + return resolved } -// ParseLockfileVersion extracts the lockfileVersion value from pnpm-lock.yaml content -// using proper YAML parsing. -func ParseLockfileVersion(content []byte) string { - var doc yaml.Node - if err := yaml.Unmarshal(content, &doc); err != nil { - return "" - } - if doc.Kind != yaml.DocumentNode || len(doc.Content) == 0 { - return "" +func mergeImporterDeps(entry ImporterEntry) map[string]DepRef { + result := make(map[string]DepRef) + for name, ref := range entry.Dependencies { + result[name] = ref } - mapping := doc.Content[0] - if mapping.Kind != yaml.MappingNode { - return "" + for name, ref := range entry.DevDependencies { + result[name] = ref } - for i := 0; i < len(mapping.Content)-1; i += 2 { - if mapping.Content[i].Value == "lockfileVersion" { - return mapping.Content[i+1].Value - } + for name, ref := range entry.OptionalDependencies { + result[name] = ref } - return "" + return result } -// parseDiffChangedLines extracts the new-file line numbers of changed lines from a unified diff. -func parseDiffChangedLines(diffText string) []int { - var result []int - lines := strings.Split(diffText, "\n") +// hasTransitiveChanges checks if any package in the transitive closure of +// startKey has different snapshot entries between old and new lockfiles. +func hasTransitiveChanges(startKey string, oldSnapshots, newSnapshots map[string]SnapshotEntry) bool { + visited := make(map[string]bool) + queue := []string{startKey} - newLineNum := 0 - for _, line := range lines { - if strings.HasPrefix(line, "@@") { - plusIdx := strings.Index(line, "+") - if plusIdx < 0 { - continue - } - rest := line[plusIdx+1:] - spaceIdx := strings.Index(rest, " ") - if spaceIdx < 0 { - continue - } - rangeStr := rest[:spaceIdx] - parts := strings.SplitN(rangeStr, ",", 2) - start, err := strconv.Atoi(parts[0]) - if err != nil { - continue - } - newLineNum = start - 1 + for len(queue) > 0 { + key := queue[0] + queue = queue[1:] + + if visited[key] { continue } + visited[key] = true + + oldEntry, oldExists := oldSnapshots[key] + newEntry, newExists := newSnapshots[key] - if newLineNum == 0 { + if oldExists != newExists { + return true + } + if !newExists { continue } - if strings.HasPrefix(line, "-") { - continue + if !stringMapsEqual(oldEntry.Dependencies, newEntry.Dependencies) { + return true } + if !stringMapsEqual(oldEntry.OptionalDependencies, newEntry.OptionalDependencies) { + return true + } + + for name, version := range newEntry.Dependencies { + queue = append(queue, name+"@"+version) + } + for name, version := range newEntry.OptionalDependencies { + queue = append(queue, name+"@"+version) + } + } - newLineNum++ + return false +} - if strings.HasPrefix(line, "+") { - result = append(result, newLineNum) +func stringMapsEqual(a, b map[string]string) bool { + if len(a) != len(b) { + return false + } + for k, v := range a { + if b[k] != v { + return false } } - return result + return true } diff --git a/main.go b/main.go index 67a8bbf..cade810 100644 --- a/main.go +++ b/main.go @@ -469,6 +469,8 @@ func main() { } // findLockfileAffectedProjects checks each subspace's pnpm-lock.yaml for dep changes. +// Parses old (merge base) and new (current) lockfiles as YAML and compares resolved +// versions for direct and transitive dependencies. // Returns: // - depChanges: project folder → set of changed external dep package names // - versionChanges: subspace name → true for subspaces where lockfileVersion changed @@ -486,28 +488,21 @@ func findLockfileAffectedProjects(config *rush.Config, mergeBase string) (map[st versionChanged := make(map[string]bool) for subspace := range subspaces { lockfilePath := filepath.Join("common", "config", "subspaces", subspace, "pnpm-lock.yaml") - if _, err := os.Stat(lockfilePath); err != nil { - continue - } - - // Compare lockfileVersion between base commit and current newContent, err := os.ReadFile(lockfilePath) if err != nil { continue } oldContent, _ := git.ShowFile(mergeBase, lockfilePath) - oldVersion := lockfile.ParseLockfileVersion([]byte(oldContent)) - newVersion := lockfile.ParseLockfileVersion(newContent) - if oldVersion != newVersion { + + oldLf := lockfile.ParseLockfile([]byte(oldContent)) + newLf := lockfile.ParseLockfile(newContent) + + if oldLf.Version() != newLf.Version() { versionChanged[subspace] = true - logf("lockfileVersion changed in subspace %q: %q → %q\n", subspace, oldVersion, newVersion) + logf("lockfileVersion changed in subspace %q: %q → %q\n", subspace, oldLf.Version(), newLf.Version()) } - diffText, err := git.DiffSincePath(mergeBase, lockfilePath) - if err != nil || diffText == "" { - continue - } - affected := lockfile.FindDepAffectedProjects(lockfilePath, subspace, diffText) + affected := lockfile.FindDepChanges(oldLf, newLf, subspace) for folder, deps := range affected { if result[folder] == nil { result[folder] = make(map[string]bool)