diff --git a/acceptance/localenv/merge-warnings-json/out.test.toml b/acceptance/localenv/merge-warnings-json/out.test.toml new file mode 100644 index 0000000000..e90b6d5d1b --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/localenv/merge-warnings-json/output.txt b/acceptance/localenv/merge-warnings-json/output.txt new file mode 100644 index 0000000000..eb92a948c8 --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/output.txt @@ -0,0 +1,76 @@ + +>>> [CLI] environments setup-local --serverless-version 4 --dry-run --output json +{ + "schemaVersion": 1, + "command": "environments setup-local", + "ok": true, + "mode": "default", + "dryRun": true, + "compute": { + "source": "serverless", + "serverlessVersion": "v4", + "envKey": "serverless/serverless-v4" + }, + "resolved": { + "pythonVersion": "3.12", + "dbconnectVersion": "17.2.0", + "artifactSource": "network" + }, + "greenfield": false, + "plan": { + "wouldWrite": "[TEST_TMP_DIR]/pyproject.toml", + "wouldBackup": "[TEST_TMP_DIR]/pyproject.toml.bak", + "wouldInstallPython": "3.12", + "diff": "--- pyproject.toml\n+++ pyproject.toml.new\n@@ -1,9 +1,17 @@\n [project]\n name = \"demo\"\n-requires-python = \"\u003e=3.10\"\n+requires-python = \"\u003e=3.12\"\n dependencies = [\"pyarrow==21.0.0\"]\n \n [dependency-groups]\n-dev = [\"databricks-connect~=16.0.0\", {include-group = \"spark\"}]\n+dev = [\"databricks-connect~=17.2.0\", {include-group = \"spark\"}]\n spark = [\"databricks-connect==15.0.0\"]\n qa = [\"pandas==4.0.0\"]\n+\n+# managed by databricks environments setup-local — do not edit\n+[tool.uv]\n+constraint-dependencies = [\n+ \"pyarrow\u003c19\",\n+ \"pandas\u003c3\",\n+]\n+# end managed by databricks environments setup-local\n" + }, + "phases": [ + { + "phase": "preflight", + "status": "ok" + }, + { + "phase": "resolve", + "status": "ok" + }, + { + "phase": "fetch", + "status": "ok" + }, + { + "phase": "merge", + "status": "ok" + }, + { + "phase": "provision", + "status": "ok" + }, + { + "phase": "validate", + "status": "ok" + } + ], + "warnings": [ + { + "code": "W_REQUIRES_PYTHON_OVERRIDDEN", + "message": "requires-python \"\u003e=3.10\" is replaced by the environment's \"\u003e=3.12\"" + }, + { + "code": "W_DBCONNECT_PIN_OVERRIDDEN", + "message": "databricks-connect \"databricks-connect~=16.0.0\" is replaced by the environment's \"databricks-connect~=17.2.0\"" + }, + { + "code": "W_DBCONNECT_PIN_DUPLICATED", + "message": "databricks-connect \"databricks-connect==15.0.0\" is not rewritten by the merge; the environment's \"databricks-connect~=17.2.0\" sits in \"dev\" alongside it, and no version satisfies both" + }, + { + "code": "W_USER_CONSTRAINT_CONFLICT", + "message": "dependency \"pyarrow==21.0.0\" conflicts with the environment constraint \"pyarrow\u003c19\"" + }, + { + "code": "W_USER_CONSTRAINT_CONFLICT", + "message": "dependency \"pandas==4.0.0\" conflicts with the environment constraint \"pandas\u003c3\"" + } + ], + "error": null, + "durationMs": 0 +} diff --git a/acceptance/localenv/merge-warnings-json/script b/acceptance/localenv/merge-warnings-json/script new file mode 100644 index 0000000000..303d347527 --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/script @@ -0,0 +1,16 @@ +# An existing project whose pins the merge overrides, plus a dependency the env's +# constraints exclude and a pin in a non-dev group uv still locks. All four warning +# conditions in one run, so warnings[] is exercised end to end. +cat > pyproject.toml <<'PY' +[project] +name = "demo" +requires-python = ">=3.10" +dependencies = ["pyarrow==21.0.0"] + +[dependency-groups] +dev = ["databricks-connect~=16.0.0", {include-group = "spark"}] +spark = ["databricks-connect==15.0.0"] +qa = ["pandas==4.0.0"] +PY + +trace $CLI environments setup-local --serverless-version 4 --dry-run --output json diff --git a/acceptance/localenv/merge-warnings-json/test.toml b/acceptance/localenv/merge-warnings-json/test.toml new file mode 100644 index 0000000000..e22bb0b151 --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/test.toml @@ -0,0 +1,24 @@ +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +# The script writes pyproject.toml as the merge input; --dry-run leaves it unchanged. +Ignore = ["pyproject.toml"] + +[Env] +DATABRICKS_LOCALENV_CONSTRAINT_SOURCE_URL_TEST_OVERRIDE = "$DATABRICKS_HOST" + +[[Server]] +Pattern = "GET /serverless/serverless-v4/pyproject.toml" +Response.Body = ''' +[project] +requires-python = ">=3.12" + +[dependency-groups] +dev = ["databricks-connect~=17.2.0"] + +[tool.uv] +constraint-dependencies = ["pyarrow<19", "pandas<3"] +''' + +[[Repls]] +Old = 'uv uv \S+(?: \([^)]+\))?' +New = 'uv [UV_VERSION]' diff --git a/acceptance/localenv/merge-warnings/out.test.toml b/acceptance/localenv/merge-warnings/out.test.toml new file mode 100644 index 0000000000..e90b6d5d1b --- /dev/null +++ b/acceptance/localenv/merge-warnings/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/localenv/merge-warnings/output.txt b/acceptance/localenv/merge-warnings/output.txt new file mode 100644 index 0000000000..fd719fb5dd --- /dev/null +++ b/acceptance/localenv/merge-warnings/output.txt @@ -0,0 +1,18 @@ + +>>> [CLI] environments setup-local --serverless-version 4 --dry-run +preflight ok check +resolve ok source=serverless envKey=serverless/serverless-v4 +fetch ok source=[DATABRICKS_URL]/serverless/serverless-v4/pyproject.toml fromCache=false +merge ok +provision ok +validate ok +warning: requires-python ">=3.10" is replaced by the environment's ">=3.12" +warning: databricks-connect "databricks-connect~=16.0.0" is replaced by the environment's "databricks-connect~=17.2.0" +warning: databricks-connect "databricks-connect==15.0.0" is not rewritten by the merge; the environment's "databricks-connect~=17.2.0" sits in "dev" alongside it, and no version satisfies both +warning: dependency "pyarrow==21.0.0" conflicts with the environment constraint "pyarrow<19" +warning: dependency "pandas==4.0.0" conflicts with the environment constraint "pandas<3" +Plan: [TEST_TMP_DIR]/pyproject.toml + changed region: requires-python + changed region: databricks-connect + changed region: tool.uv.constraint-dependencies +Check complete. No files were modified. diff --git a/acceptance/localenv/merge-warnings/script b/acceptance/localenv/merge-warnings/script new file mode 100644 index 0000000000..e1f60b9670 --- /dev/null +++ b/acceptance/localenv/merge-warnings/script @@ -0,0 +1,15 @@ +# The text renderer's warning lines, which the --json variant does not exercise. +# See merge-warnings-json for the same input through --output json. +cat > pyproject.toml <<'PY' +[project] +name = "demo" +requires-python = ">=3.10" +dependencies = ["pyarrow==21.0.0"] + +[dependency-groups] +dev = ["databricks-connect~=16.0.0", {include-group = "spark"}] +spark = ["databricks-connect==15.0.0"] +qa = ["pandas==4.0.0"] +PY + +trace $CLI environments setup-local --serverless-version 4 --dry-run diff --git a/acceptance/localenv/merge-warnings/test.toml b/acceptance/localenv/merge-warnings/test.toml new file mode 100644 index 0000000000..e22bb0b151 --- /dev/null +++ b/acceptance/localenv/merge-warnings/test.toml @@ -0,0 +1,24 @@ +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +# The script writes pyproject.toml as the merge input; --dry-run leaves it unchanged. +Ignore = ["pyproject.toml"] + +[Env] +DATABRICKS_LOCALENV_CONSTRAINT_SOURCE_URL_TEST_OVERRIDE = "$DATABRICKS_HOST" + +[[Server]] +Pattern = "GET /serverless/serverless-v4/pyproject.toml" +Response.Body = ''' +[project] +requires-python = ">=3.12" + +[dependency-groups] +dev = ["databricks-connect~=17.2.0"] + +[tool.uv] +constraint-dependencies = ["pyarrow<19", "pandas<3"] +''' + +[[Repls]] +Old = 'uv uv \S+(?: \([^)]+\))?' +New = 'uv [UV_VERSION]' diff --git a/libs/localenv/merge.go b/libs/localenv/merge.go index f8a8a88e45..b48b60535a 100644 --- a/libs/localenv/merge.go +++ b/libs/localenv/merge.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "regexp" + "slices" "strings" ) @@ -44,6 +45,30 @@ var ( requiresPythonRe = regexp.MustCompile(`^(\s*)requires-python\s*=`) ) +// replacedDBConnectPin returns the databricks-connect requirement that merging +// target would rewrite in place, or "" when the merge would instead insert the +// managed pin (leaving anything already there untouched). +// +// detectMergeWarnings uses this rather than re-deciding which pins the merge +// recognizes: the merge matches double-quoted elements within the array devKeyRe +// finds, and any second implementation of that rule drifts from it. A detector that +// disagrees reports an override for a pin the merge never touched, which is a false +// claim about the user's file rather than a missed warning. +func replacedDBConnectPin(target []byte, c Constraints) string { + if c.DatabricksConnect == "" { + return "" + } + // Mirror MergeManaged's own preprocessing so the same lines are inspected. + lines := strings.Split(strings.ReplaceAll(string(target), "\r\n", "\n"), "\n") + if containsMultilineString(lines) { + return "" + } + // mergeDatabricksConnect rewrites element lines in place, so hand it a copy: this + // probe must not disturb the caller's view of the pre-merge file. + _, replaced, _ := mergeDatabricksConnect(slices.Clone(lines), c.DatabricksConnect) + return replaced +} + // MergeManaged applies the three managed transforms to target, preserving every other // byte (comments, ordering, whitespace). It returns the merged bytes and the list of // regions that actually changed. The operation is idempotent: feeding its own output @@ -87,7 +112,7 @@ func MergeManaged(target []byte, c Constraints) (merged []byte, regions []string regions = append(regions, regionRequiresPython) } - lines, dbcChanged := mergeDatabricksConnect(lines, c.DatabricksConnect) + lines, _, dbcChanged := mergeDatabricksConnect(lines, c.DatabricksConnect) if dbcChanged { regions = append(regions, regionDatabricksConnect) } @@ -237,16 +262,21 @@ var devKeyRe = regexp.MustCompile(`^\s*dev\s*=`) // databricks-connect pin sitting in a sibling group (e.g. docs/test) or inside a // trailing comment on some other line is never clobbered. The insert path is // idempotent: a subsequent merge finds the element and rewrites it in place. -func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { +// +// replacedPin is the requirement it rewrote in place, empty when it inserted the +// managed pin instead. That distinction is what detectMergeWarnings needs: only a +// rewrite means the user's pin is gone, and only the merge itself can say which +// spellings it recognizes. +func mergeDatabricksConnect(lines []string, value string) (out []string, replacedPin string, changed bool) { if value == "" { - return lines, false + return lines, "", false } elem := `"` + value + `"` header, end, found := tableBounds(lines, "[dependency-groups]") if !found { // No [dependency-groups] table: append a fresh managed dev group. - return appendManagedBlock(lines, []string{"[dependency-groups]", "dev = [", " " + elem + ",", "]"}), true + return appendManagedBlock(lines, []string{"[dependency-groups]", "dev = [", " " + elem + ",", "]"}), "", true } // Locate the dev assignment and the line span of its array value. @@ -264,7 +294,7 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { out = append(out, lines[:header+1]...) out = append(out, insert...) out = append(out, lines[header+1:]...) - return out, true + return out, "", true } arrayLast, _ := arrayLineSpan(lines, devStart, end) @@ -274,19 +304,19 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { if devStart == arrayLast { line := lines[devStart] arrayPart, commentPart := splitAtArrayClose(line) - if replaced, ok := replaceDbconnectElement(arrayPart, elem); ok { - newLine := replaced + commentPart + if rewritten, replaced, ok := replaceDbconnectElement(arrayPart, elem); ok { + newLine := rewritten + commentPart if newLine == line { - return lines, false + return lines, replaced, false } lines[devStart] = newLine - return lines, true + return lines, replaced, true } // No databricks-connect element: insert one as the first array element. open := strings.Index(arrayPart, "[") closeIdx := strings.LastIndex(arrayPart, "]") if open < 0 || closeIdx < open { - return lines, false + return lines, "", false } inner := strings.TrimSpace(arrayPart[open+1 : closeIdx]) newInner := elem @@ -294,7 +324,7 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { newInner = elem + ", " + inner } lines[devStart] = arrayPart[:open+1] + newInner + arrayPart[closeIdx:] + commentPart - return lines, true + return lines, "", true } // Multi-line form: the array spans devStart..arrayLast. An existing @@ -310,15 +340,15 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { if c := commentStart(code); c >= 0 { code, comment = code[:c], code[c:] } - if replaced, ok := replaceDbconnectElement(code, elem); ok { + if rewritten, replaced, ok := replaceDbconnectElement(code, elem); ok { // Rewrite only the code portion; a trailing comment is user content and // must be preserved byte-for-byte, even if it contains a quoted token. - newLine := replaced + comment + newLine := rewritten + comment if newLine == lines[i] { - return lines, false + return lines, replaced, false } lines[i] = newLine - return lines, true + return lines, replaced, true } if dbconnectQuotedRe.MatchString(code) { lastElem = i @@ -340,11 +370,11 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { if lastElem >= 0 && lastElem < arrayLast { lines[lastElem] = ensureTrailingComma(lines[lastElem]) } - out := make([]string, 0, len(lines)+1) - out = append(out, lines[:arrayLast]...) - out = append(out, indent+elem+",") - out = append(out, lines[arrayLast:]...) - return out, true + inserted := make([]string, 0, len(lines)+1) + inserted = append(inserted, lines[:arrayLast]...) + inserted = append(inserted, indent+elem+",") + inserted = append(inserted, lines[arrayLast:]...) + return inserted, "", true } // dbconnectQuotedRe matches any double-quoted array element token. @@ -353,18 +383,18 @@ var dbconnectQuotedRe = regexp.MustCompile(`"[^"]*"`) // replaceDbconnectElement replaces the first quoted element in code whose package // name is databricks-connect (compared under PEP 503 normalization, so // "databricks_connect" / "Databricks-Connect" / "databricks.connect" all match) -// with elem. It returns the rewritten code and whether a replacement was made. -// Matching mirrors the artifact side (isDatabricksConnectDep) so a differently -// spelled existing pin is rewritten in place rather than left for the insert path -// to duplicate. -func replaceDbconnectElement(code, elem string) (string, bool) { +// with elem. It returns the rewritten code, the requirement it replaced, and +// whether a replacement was made. Matching mirrors the artifact side +// (isDatabricksConnectDep) so a differently spelled existing pin is rewritten in +// place rather than left for the insert path to duplicate. +func replaceDbconnectElement(code, elem string) (out, replaced string, ok bool) { for _, m := range dbconnectQuotedRe.FindAllStringIndex(code, -1) { inner := code[m[0]+1 : m[1]-1] if isDatabricksConnectDep(inner) { - return code[:m[0]] + elem + code[m[1]:], true + return code[:m[0]] + elem + code[m[1]:], inner, true } } - return code, false + return code, "", false } // ensureTrailingComma appends a "," after the last non-space code character of diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 30f803a21e..d887e92dd7 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -307,6 +307,14 @@ func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, if err != nil { return nil, greenfield, p.fail(PhaseMerge, false, NewError(ErrMerge, err, "merge managed regions failed")) } + // Surface merge-quality warnings (overridden or duplicated pins, conflicting + // user constraints) from the pre-merge file. Greenfield has nothing of the + // user's to override, so it is skipped. This runs for both dry-run and real + // runs so the --json consumer sees the same warnings either way. The pin the + // merge rewrote comes from the merge itself, so the warning can never claim a + // replacement that did not happen. + p.res.Warnings = append(p.res.Warnings, + detectMergeWarnings(baseBytes, effective, replacedDBConnectPin(baseBytes, effective))...) } // Under --dry-run, build the plan (with a diff) for reporting. A real run does diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 865e4e3026..bdbf975322 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -156,6 +156,43 @@ func TestPipelineCheckMutatesNothing(t *testing.T) { assert.Empty(t, entries) } +func TestPipelineSurfacesMergeWarnings(t *testing.T) { + // writeProject pins requires-python ">=3.10" and databricks-connect ~=16.0.0, + // while sampleToml pins "==3.12.*" and ~=17.2.0 — the merge overrides both, so the + // result must carry both override warnings. + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.Equal(t, []string{WarnRequiresPythonOverridden, WarnDBConnectPinOverridden}, codes(res.Warnings)) +} + +func TestPipelineGreenfieldHasNoWarnings(t *testing.T) { + // A greenfield project has nothing of the user's to override. + dir := t.TempDir() + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.Empty(t, res.Warnings) + assert.True(t, res.Greenfield) +} + func TestPipelineReportsCancellationNotProvisionFailure(t *testing.T) { // When the context is cancelled mid-provision (a Ctrl-C / SIGTERM), the run // must surface E_CANCELED, not E_PROVISION — the provision phase's own error diff --git a/libs/localenv/result.go b/libs/localenv/result.go index 244769727a..5a41f5faab 100644 --- a/libs/localenv/result.go +++ b/libs/localenv/result.go @@ -189,12 +189,46 @@ type PhaseStatus struct { Detail string `json:"-"` } -// Warning is a non-fatal advisory surfaced in --json "warnings" (spec §6). +// Warning is a non-fatal advisory surfaced in --json "warnings" (spec §6). Code +// is a stable, categorical identifier from the closed set below; Message is +// human-readable text for the text renderer and is not part of the contract. type Warning struct { Code string `json:"code"` Message string `json:"message"` } +// Warning codes are the closed, categorical set surfaced in --json warnings[]. +// They let a consumer report a count and a code histogram (merge quality) without +// parsing free-form text. All are emitted from the merge phase, where the fetched +// env-owned pins can conflict with what the user already had. +// +// The messages are phrased in the present tense because the same detection runs +// under --dry-run, where nothing has been written yet. +const ( + // WarnRequiresPythonOverridden: the user's [project].requires-python differs + // from the env's pin and is replaced by the managed value. + WarnRequiresPythonOverridden = "W_REQUIRES_PYTHON_OVERRIDDEN" + // WarnDBConnectPinOverridden: the user's databricks-connect pin sits directly in + // the dev group and is replaced by the managed value. + WarnDBConnectPinOverridden = "W_DBCONNECT_PIN_OVERRIDDEN" + // WarnDBConnectPinDuplicated: a databricks-connect pin of the user's is one the + // merge does not rewrite — reached through a PEP 735 include-group, a second pin + // in the dev array, or a group key that normalizes to "dev" without matching the + // literal key the merge edits — so the managed pin lands in the dev group + // alongside it. Unlike an override this leaves two pins for one package and uv + // cannot resolve it, a distinct and worse outcome that needs a manual fix, so it + // carries its own code. It can accompany an override and persists across re-runs + // for as long as the retained pin does. + WarnDBConnectPinDuplicated = "W_DBCONNECT_PIN_DUPLICATED" + // WarnUserConstraintConflict: a user dependency pins a package that the env's + // constraint-dependencies also constrains, to a provably non-overlapping version + // range (uv will fail to resolve). Every requirement uv locks is scanned — + // [project].dependencies, the optional-dependency extras, and all dependency + // groups — since constraint-dependencies applies to the whole resolution. Emitted + // only when the ranges are provably disjoint; ambiguous cases are not flagged. + WarnUserConstraintConflict = "W_USER_CONSTRAINT_CONFLICT" +) + // Result is the full outcome of a sync run and the root of the --json object // (spec §6). Field order matches the spec's schema so JSON key order is stable. // diff --git a/libs/localenv/warnings.go b/libs/localenv/warnings.go new file mode 100644 index 0000000000..3eb5821a2d --- /dev/null +++ b/libs/localenv/warnings.go @@ -0,0 +1,533 @@ +package localenv + +import ( + "fmt" + "maps" + "regexp" + "slices" + "strconv" + "strings" + + "github.com/BurntSushi/toml" +) + +// depSpecRe splits a dependency string into its package name and the remainder +// (e.g. "pyarrow~=19.0" -> "pyarrow", "~=19.0"). It stops the name at the first +// PEP 508 separator; splitDepSpec handles the extras and markers that may lead the +// remainder. +var depSpecRe = regexp.MustCompile(`^([A-Za-z0-9._-]+)\s*(.*)$`) + +// singleClauseRe parses one version clause: an operator and a dotted numeric +// release. Pre/post/dev suffixes and wildcards are not modeled — a clause we +// cannot parse this simply is treated as "unknown" and never yields a conflict. +var singleClauseRe = regexp.MustCompile(`^(>=|<=|==|~=|!=|<|>)?\s*([0-9]+(?:\.[0-9]+)*)`) + +// splitDepSpec returns the normalized package name and the version specifier +// portion of a dependency string, with any extras stripped. ok is false when there +// is no recognizable name, or when what follows the name carries an environment +// marker (see below). +func splitDepSpec(dep string) (name, spec string, ok bool) { + m := depSpecRe.FindStringSubmatch(strings.TrimSpace(dep)) + if m == nil { + return "", "", false + } + // Extras select optional features and never narrow the version range, so drop + // them to expose the specifier underneath ("pkg[a,b]==1.0" -> "==1.0"). + spec = strings.TrimSpace(m[2]) + if strings.HasPrefix(spec, "[") { + if i := strings.Index(spec, "]"); i >= 0 { + spec = strings.TrimSpace(spec[i+1:]) + } + } + // A marker gates the whole requirement on the resolving interpreter, which we do + // not evaluate, so a pin that may not apply must not be compared. Checking the + // remainder rather than the raw string scopes this to where a marker can actually + // appear, leaving an extras list that happens to contain ";" alone. A ";" inside + // a url requirement still lands here, which costs nothing: a url has no + // comparable version range and would be undecidable regardless. + if strings.Contains(spec, ";") { + return "", "", false + } + return normalizePackageName(m[1]), spec, true +} + +// userPyprojectTOML is a permissive view of the fields detectMergeWarnings reads +// from the *user's* pyproject.toml. +// +// It deliberately does not reuse pyprojectTOML: that struct decodes the +// Databricks-owned constraint artifact, whose shape we control, whereas a user's +// file may legitimately carry shapes a stricter struct rejects — a PEP 735 table +// entry such as {include-group = "test"} beside requirement strings, a PEP 621 +// dependency table, or a group declared as a sub-table ([dependency-groups.docs], +// the PDM/mkdocs style). BurntSushi reports a type mismatch on any single key as a +// whole-document error, so one such shape anywhere would drop every warning, +// including the ones that do not read the offending key. The container fields are +// therefore typed as []any / map[string]any and narrowed at the point of use. +// requires-python stays a string because PEP 621 specifies it as one; a non-string +// there is malformed and uv rejects the file outright. +type userPyprojectTOML struct { + Project struct { + RequiresPython string `toml:"requires-python"` + Dependencies []any `toml:"dependencies"` + // uv resolves extras alongside the base dependencies, so a pin here is + // subject to constraint-dependencies exactly like a [project] one. + OptionalDependencies map[string]any `toml:"optional-dependencies"` + } `toml:"project"` + // Every group is decoded, not just dev: uv locks all declared groups, so a pin in + // any of them is subject to constraint-dependencies (see resolutionRequirements). + DependencyGroups map[string]any `toml:"dependency-groups"` +} + +// devGroup is the dependency group whose databricks-connect pin the merge manages. +const devGroup = "dev" + +// stringEntries returns the requirement strings in a decoded dependency array, +// skipping entries of any other shape (a PEP 735 include-group table, a PEP 621 +// dependency table). A value that is not an array at all yields nothing. +func stringEntries(v any) []string { + entries, ok := v.([]any) + if !ok { + return nil + } + var out []string + for _, e := range entries { + if s, ok := e.(string); ok { + out = append(out, s) + } + } + return out +} + +// retainedRequirements returns every requirement uv resolves that the merge leaves +// in place: all of resolutionRequirements minus the single entry the merge rewrites, +// identified by replacedPin. +// +// Deriving it by subtraction rather than by re-deciding which groups and spellings +// the merge edits keeps the two from disagreeing: whatever the merge does not report +// as replaced is, by definition, still in the file next to the managed pin. Only the +// first occurrence is dropped, because the merge rewrites only the first element — +// a second identical pin genuinely survives. +func retainedRequirements(p userPyprojectTOML, replacedPin string) []string { + all := resolutionRequirements(p) + if replacedPin == "" { + return all + } + out := make([]string, 0, len(all)) + dropped := false + for _, r := range all { + if !dropped && strings.TrimSpace(r) == strings.TrimSpace(replacedPin) { + dropped = true + continue + } + out = append(out, r) + } + return out +} + +// resolutionRequirements returns every requirement string uv considers when it locks +// the project, in a deterministic order: [project].dependencies, then each extra in +// [project.optional-dependencies], then each dependency group — all of them, not +// just dev. +// +// Scanning every group is what makes the conflict warning match uv's behaviour: +// constraint-dependencies applies to the whole resolution, so a pin in any declared +// group fails `uv sync` with "requirements are unsatisfiable" just as a [project] +// one does. Restricting the scan to dev would stay silent on exactly that failure. +// Every group is visited on its own, so a PEP 735 {include-group = "..."} reference +// needs no traversal: the group it names is enumerated here regardless, and an +// include-group cycle cannot cause recursion. +func resolutionRequirements(p userPyprojectTOML) []string { + out := stringEntries(p.Project.Dependencies) + for _, extra := range slices.Sorted(maps.Keys(p.Project.OptionalDependencies)) { + out = append(out, stringEntries(p.Project.OptionalDependencies[extra])...) + } + for _, g := range slices.Sorted(maps.Keys(p.DependencyGroups)) { + out = append(out, stringEntries(p.DependencyGroups[g])...) + } + return out +} + +// detectMergeWarnings compares a user's existing pyproject.toml against the +// fetched env constraints and returns the categorical warnings the merge phase +// surfaces (spec §6 warnings[]). It is a read-only comparison run alongside the +// merge; MergeManaged still owns the actual byte edits. +// +// It is best-effort: unparseable input yields no warnings rather than an error, +// because a warning is advisory and must never fail the run. The user's file is +// decoded through userPyprojectTOML rather than the artifact's stricter +// pyprojectTOML, so no shape a real pyproject.toml may legitimately carry can +// suppress the checks that do not depend on it. Greenfield projects (no pre-existing +// content) produce nothing — there is nothing of the user's to override. Warnings are +// deterministic and ordered (requires-python, then databricks-connect, then +// constraint conflicts in the order uv would encounter them) so goldens are stable. +func detectMergeWarnings(userPyproject []byte, c Constraints, replacedPin string) []Warning { + if len(userPyproject) == 0 { + return nil + } + // A decode error is not fatal: userPyprojectTOML types its containers loosely so + // the shapes a user's file legitimately carries all decode, leaving genuine syntax + // errors as the only failure — and those yield no fields to compare anyway. + var p userPyprojectTOML + if err := toml.Unmarshal(userPyproject, &p); err != nil { + return nil + } + + var warnings []Warning + + // The user pinned a requires-python that differs from the env's pin; the merge + // replaces it with the managed value. + if up := strings.TrimSpace(p.Project.RequiresPython); up != "" && c.RequiresPython != "" && up != strings.TrimSpace(c.RequiresPython) { + warnings = append(warnings, Warning{ + Code: WarnRequiresPythonOverridden, + Message: fmt.Sprintf("requires-python %q is replaced by the environment's %q", up, c.RequiresPython), + }) + } + + // The user's databricks-connect pin differs from the env's. Only meaningful in + // default mode (c.DatabricksConnect is empty in constraints-only, where the dev + // group is left untouched). + if c.DatabricksConnect != "" { + warnings = append(warnings, dbconnectWarnings(p, replacedPin, c.DatabricksConnect)...) + } + + // The pin the merge replaces is excluded from the conflict scan: a conflict against + // an entry that is about to be discarded describes a state that does not survive + // the merge, and its own warning already covers it. Retained databricks-connect + // pins stay in scope — they are still part of the resolution. + warnings = append(warnings, constraintConflicts(retainedRequirements(p, replacedPin), c.ConstraintDeps)...) + return warnings +} + +// dbconnectWarnings reports how the merge treats the user's databricks-connect +// pins, distinguishing two outcomes that need different user actions. +// +// Which pin the merge rewrites is not re-derived here — replacedPin comes from the +// merge itself (see replacedDBConnectPin). Every other databricks-connect +// requirement uv would resolve is left in place beside the managed pin: a pin in a +// group the merge does not edit, one reached through a PEP 735 include-group, or a +// second element in dev's own array. That leaves two pins for one package, and where +// their ranges do not intersect uv cannot resolve at all — a strictly worse outcome +// than an override, and one the user has to fix by hand. Reporting it as "is +// replaced" would state something factually untrue and hide the resolution failure +// behind a reassuring advisory. +// +// The two conditions are reported independently rather than as a first-match choice, +// because they can coexist and the duplicate outlives the override: once the env's +// pin is in dev, a re-run finds a matching direct pin, and stopping there would go +// silent on a retained pin that still makes the project unresolvable. +func dbconnectWarnings(p userPyprojectTOML, replacedPin, envPin string) []Warning { + var warnings []Warning + + envPin = strings.TrimSpace(envPin) + if replacedPin != "" && strings.TrimSpace(replacedPin) != envPin { + warnings = append(warnings, Warning{ + Code: WarnDBConnectPinOverridden, + Message: fmt.Sprintf("databricks-connect %q is replaced by the environment's %q", strings.TrimSpace(replacedPin), envPin), + }) + } + + _, envSpec, envOK := splitDepSpec(envPin) + for _, pin := range dbconnectPins(retainedRequirements(p, replacedPin)) { + if pin == envPin { + // An identical pin needs no reconciliation: uv sees one requirement twice. + continue + } + // Two pins are only a problem when nothing satisfies both. ">=16" beside + // "~=17.2.0" resolves at 17.2.x, so there is nothing for the user to do, and + // claiming otherwise would send them after a non-existent conflict. + _, pinSpec, pinOK := splitDepSpec(pin) + if !envOK || !pinOK || !rangesDisjoint(pinSpec, envSpec) { + continue + } + warnings = append(warnings, Warning{ + Code: WarnDBConnectPinDuplicated, + Message: fmt.Sprintf("databricks-connect %q is not rewritten by the merge; the environment's %q sits in %q alongside it, and no version satisfies both", + pin, envPin, devGroup), + }) + } + return warnings +} + +// dbconnectPins returns the trimmed databricks-connect requirements among entries, +// in order. +func dbconnectPins(entries []string) []string { + var out []string + for _, e := range entries { + if isDatabricksConnectDep(e) { + out = append(out, strings.TrimSpace(e)) + } + } + return out +} + +// constraintConflicts flags each user dependency pin that the env's +// constraint-dependencies also constrains to a provably non-overlapping version. +// It is deliberately conservative — it only fires when the two ranges are provably +// disjoint (see rangesDisjoint); an ambiguous pair yields nothing, so a false +// "conflict" is never reported. Warnings come out in userDeps order, and one +// requirement reported once however many places declare it: the same pin listed in +// [project].dependencies and in a group is a single conflict to fix, and repeating it +// would inflate the code histogram consumers build from warnings[]. +func constraintConflicts(userDeps, envConstraints []string) []Warning { + if len(userDeps) == 0 || len(envConstraints) == 0 { + return nil + } + // Index the env constraints by normalized package name. Multiple entries for one + // package compose as a conjunction, so they are joined with "," rather than + // letting the last one win, which would decide against an arbitrary subset of the + // clauses depending on artifact ordering. specInterval intersects the clauses, so + // the joined form is evaluated as the conjunction it is. + envByName := make(map[string]string, len(envConstraints)) + for _, ec := range envConstraints { + name, spec, ok := splitDepSpec(ec) + if !ok || spec == "" { + continue + } + if prev, dup := envByName[name]; dup { + spec = prev + "," + spec + } + envByName[name] = spec + } + if len(envByName) == 0 { + return nil + } + + var warnings []Warning + reported := make(map[string]bool) + for _, ud := range userDeps { + name, userSpec, ok := splitDepSpec(ud) + if !ok || userSpec == "" { + continue + } + envSpec, ok := envByName[name] + if !ok { + continue + } + if !rangesDisjoint(userSpec, envSpec) { + continue + } + // Keyed on the normalized name and the spec, so two spellings of one pin + // ("pyarrow==21" and "PyArrow ==21") collapse, while genuinely different + // conflicting pins for one package are each reported. + key := name + userSpec + if reported[key] { + continue + } + reported[key] = true + warnings = append(warnings, Warning{ + Code: WarnUserConstraintConflict, + Message: fmt.Sprintf("dependency %q conflicts with the environment constraint %q", strings.TrimSpace(ud), name+envSpec), + }) + } + return warnings +} + +// clause is a parsed single version clause: an operator and a numeric release. +type clause struct { + op string + rel []int +} + +// parseClause parses one "" clause. ok is false when the clause cannot +// be parsed simply, which is treated as "unknown range" and never yields a conflict. +func parseClause(spec string) (clause, bool) { + spec = strings.TrimSpace(spec) + if spec == "" { + return clause{}, false + } + m := singleClauseRe.FindStringSubmatch(spec) + if m == nil { + return clause{}, false + } + // Reject anything trailing the numeric release (wildcards, pre/post/dev tags): + // modeling those correctly is out of scope, and guessing risks a false conflict. + if strings.TrimSpace(spec[len(m[0]):]) != "" { + return clause{}, false + } + op := m[1] + if op == "" { + op = "==" // a bare version in dependencies means an exact pin + } + var rel []int + for part := range strings.SplitSeq(m[2], ".") { + n, err := strconv.Atoi(part) + if err != nil { + return clause{}, false + } + rel = append(rel, n) + } + // PEP 440 requires at least two release segments after "~=" (it expands to + // ">=X.Y, ==X.*", which needs a segment to hold fixed). A single-segment base has + // no defined range, and clause.interval would index rel[:len(rel)-1] to build the + // ceiling — yielding an empty prefix and a nonsense bound. Refuse to parse it so + // it stays an unknown range rather than a wrong one. + if op == "~=" && len(rel) < 2 { + return clause{}, false + } + return clause{op: op, rel: rel}, true +} + +// rangesDisjoint reports whether two version specifiers provably share no +// satisfying version. It is intentionally partial: any pair it cannot decide — a +// "!=", an unparseable release — returns false, so an uncertain case is never +// reported as a conflict. uv remains the real resolver; this only surfaces the +// provable clashes as an advisory. +func rangesDisjoint(userSpec, envSpec string) bool { + a, aok := specInterval(userSpec) + b, bok := specInterval(envSpec) + if !aok || !bok { + return false + } + return !a.overlaps(b) +} + +// specInterval reduces a whole PEP 440 specifier to the single release range it +// admits. A comma-separated specifier is a conjunction, so the clauses are +// intersected: ">=21,<22" is [21, 22). Compound ranges are the ordinary way to pin a +// dependency, and treating the comma as undecidable would miss the conflicts they +// cause — including the ones this detector creates for itself when it conjoins +// duplicate constraint entries for one package. +// +// ok is false when any clause is undecidable, which keeps the whole specifier +// undecidable rather than letting a partial reading of it decide a conflict. +func specInterval(spec string) (interval, bool) { + if strings.TrimSpace(spec) == "" { + return interval{}, false + } + out := interval{} + for clauseSpec := range strings.SplitSeq(spec, ",") { + c, ok := parseClause(clauseSpec) + if !ok { + return interval{}, false + } + i, ok := c.interval() + if !ok { + return interval{}, false + } + out = out.intersect(i) + } + return out, true +} + +// interval is the contiguous release range a clause admits, with each endpoint +// marked inclusive or exclusive. A nil endpoint is unbounded in that direction. +// +// Modeling each clause as an interval decides every pair of the operators we parse +// — including the opposite-direction bounds (">=20" vs "<19") and "~=" vs "~=" +// pairs the published constraint artifacts actually use — with one endpoint +// comparison instead of a per-operator-pair table. +// +// Inclusivity is tracked explicitly rather than normalizing to a half-open range, +// because releases have no well-defined successor: appending a component to make +// ">3.12" into ">=3.12.1" would wrongly exclude 3.12.0.5, which does satisfy +// ">3.12" (compareRelease zero-pads, so 3.12.0.5 > 3.12). Excluding a real member +// shrinks the interval and would report an overlapping pair as disjoint. +type interval struct { + lo []int + loIncl bool + hi []int + hiIncl bool +} + +// interval converts a clause to its release range. ok is false for an operator +// whose satisfying set is not contiguous ("!=", the complement of a point), since +// disjointness cannot then be decided by comparing endpoints. +func (c clause) interval() (interval, bool) { + switch c.op { + case "==": + // A single point. compareRelease zero-pads, so this also covers the spellings + // that denote the same release (3.12 and 3.12.0). + return interval{lo: c.rel, loIncl: true, hi: c.rel, hiIncl: true}, true + case ">=": + return interval{lo: c.rel, loIncl: true}, true + case ">": + return interval{lo: c.rel}, true + case "<=": + return interval{hi: c.rel, hiIncl: true}, true + case "<": + return interval{hi: c.rel}, true + case "~=": + // PEP 440: ~=X.Y expands to ">=X.Y, ==X.*" — the floor is the base, and the + // exclusive ceiling increments the second-to-last segment. parseClause + // guarantees at least two segments here. + ceil := make([]int, len(c.rel)-1) + copy(ceil, c.rel[:len(c.rel)-1]) + ceil[len(ceil)-1]++ + return interval{lo: c.rel, loIncl: true, hi: ceil}, true + } + return interval{}, false +} + +// overlaps reports whether two intervals share any release. They intersect iff +// each one starts at or before the other ends; an endpoint touching at equal +// versions counts only when both sides include it. +func (a interval) overlaps(b interval) bool { + return startsBeforeEnd(a.lo, a.loIncl, b.hi, b.hiIncl) && + startsBeforeEnd(b.lo, b.loIncl, a.hi, a.hiIncl) +} + +// intersect returns the range satisfying both a and b: the higher floor and the +// lower ceiling, keeping an endpoint inclusive only where both sides do. The zero +// interval is unbounded in both directions and so acts as the identity, which lets a +// conjunction be folded over its clauses. +// +// An empty result (a floor above the ceiling) is not normalized to any canonical +// form: overlaps compares endpoints, so an empty interval is already disjoint from +// every interval including itself, which is the correct reading of a specifier no +// version satisfies (">=22,<21"). +func (a interval) intersect(b interval) interval { + out := a + if a.lo == nil || (b.lo != nil && !startsBeforeEnd(b.lo, true, a.lo, true)) { + // b's floor is strictly higher, so it wins outright. + out.lo, out.loIncl = b.lo, b.loIncl + } else if b.lo != nil && compareRelease(a.lo, b.lo) == 0 { + // Equal floors: the stricter exclusivity applies. + out.loIncl = a.loIncl && b.loIncl + } + if a.hi == nil || (b.hi != nil && !startsBeforeEnd(a.hi, true, b.hi, true)) { + out.hi, out.hiIncl = b.hi, b.hiIncl + } else if b.hi != nil && compareRelease(a.hi, b.hi) == 0 { + out.hiIncl = a.hiIncl && b.hiIncl + } + return out +} + +// startsBeforeEnd reports whether a range starting at lo does not begin after a +// range ending at hi. A nil endpoint is unbounded, so the condition holds. +func startsBeforeEnd(lo []int, loIncl bool, hi []int, hiIncl bool) bool { + if lo == nil || hi == nil { + return true + } + switch cmp := compareRelease(lo, hi); { + case cmp < 0: + return true + case cmp > 0: + return false + default: + // Equal endpoints meet at exactly one version, which belongs to both ranges + // only if neither excludes it. + return loIncl && hiIncl + } +} + +// compareRelease compares two dotted numeric releases component-wise, treating a +// missing trailing component as 0 (so 3.12 == 3.12.0). Returns -1, 0, or 1. +func compareRelease(a, b []int) int { + n := max(len(a), len(b)) + for i := range n { + var ai, bi int + if i < len(a) { + ai = a[i] + } + if i < len(b) { + bi = b[i] + } + if ai != bi { + if ai < bi { + return -1 + } + return 1 + } + } + return 0 +} diff --git a/libs/localenv/warnings_test.go b/libs/localenv/warnings_test.go new file mode 100644 index 0000000000..598c040e7a --- /dev/null +++ b/libs/localenv/warnings_test.go @@ -0,0 +1,573 @@ +package localenv + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// detectWarnings runs the detector the way mergePlan does, taking the replaced pin +// from the merge itself rather than a hand-supplied value. Tests must not assert +// against a different notion of what the merge rewrites than production uses — that +// divergence is the bug this wiring exists to prevent. +func detectWarnings(userPyproject []byte, c Constraints) []Warning { + return detectMergeWarnings(userPyproject, c, replacedDBConnectPin(userPyproject, c)) +} + +// codes extracts the warning codes in order for concise assertions. +func codes(ws []Warning) []string { + out := make([]string, 0, len(ws)) + for _, w := range ws { + out = append(out, w.Code) + } + return out +} + +func TestDetectMergeWarningsGreenfieldAndEmpty(t *testing.T) { + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=18.0.0"} + // No pre-existing file: nothing of the user's to override. + assert.Nil(t, detectWarnings(nil, c)) + assert.Nil(t, detectWarnings([]byte{}, c)) + // Unparseable TOML is best-effort: no warnings, no panic. + assert.Nil(t, detectWarnings([]byte("this is : not valid toml ["), c)) +} + +func TestDetectMergeWarningsOverriddenPins(t *testing.T) { + user := []byte(`[project] +name = "demo" +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.1.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=18.0.0"} + got := detectWarnings(user, c) + assert.Equal(t, []string{WarnRequiresPythonOverridden, WarnDBConnectPinOverridden}, codes(got)) +} + +func TestDetectMergeWarningsNoOverrideWhenMatching(t *testing.T) { + // User already matches the env pins exactly — the merge is a no-op, so no + // override warnings. + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect~=18.0.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=18.0.0"} + assert.Empty(t, detectWarnings(user, c)) +} + +func TestDetectMergeWarningsConstraintsOnlyIgnoresDBConnect(t *testing.T) { + // In constraints-only mode c.DatabricksConnect is empty; the user's dev pin is + // left untouched, so it must not produce an override warning. + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect~=16.1.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: ""} + assert.Empty(t, detectWarnings(user, c)) +} + +func TestDetectMergeWarningsUserConstraintConflict(t *testing.T) { + user := []byte(`[project] +requires-python = "==3.12.*" +dependencies = ["pyarrow==21.0.0", "requests>=2.0"] +`) + // The bound shape used here matches the acceptance fixtures, which is why it is + // worth covering — but note the published artifacts are entirely "~=" (every entry + // of serverless-v4 and -v5 at the time of writing), so "~=" is the shape that + // actually has to work. TestRangesDisjoint covers the "~=" pairs directly. + c := Constraints{ + RequiresPython: "==3.12.*", + ConstraintDeps: []string{"pyarrow<19", "pandas<3"}, + } + got := detectWarnings(user, c) + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(got)) + assert.Contains(t, got[0].Message, "pyarrow") + + // The same conflict through the "~=" shape the artifacts really publish. + tilde := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow~=18.1.0"}} + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(detectWarnings(user, tilde))) +} + +func TestDetectMergeWarningsNoConflictWhenCompatible(t *testing.T) { + user := []byte(`[project] +requires-python = "==3.12.*" +dependencies = ["pyarrow==18.1.0", "pandas==2.2.0"] +`) + // Both pins sit inside the env's ceilings, so there is nothing to report. + c := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow<19", "pandas<3"}} + assert.Empty(t, detectWarnings(user, c)) +} + +func TestDetectMergeWarningsIncludeGroupDoesNotSuppress(t *testing.T) { + // A PEP 735 {include-group = ...} table is a legal dev-group entry that uv + // supports. It must not fail the decode and take every unrelated warning with + // it: the requires-python override and the pyarrow conflict below do not depend + // on the dev group at all, and MergeManaged rewrites all three regions anyway. + user := []byte(`[project] +requires-python = ">=3.9" +dependencies = ["pyarrow==19.0.0"] + +[dependency-groups] +dev = ["databricks-connect~=16.1.0", {include-group = "test"}] +test = [] +`) + c := Constraints{ + RequiresPython: "==3.12.*", + DatabricksConnect: "databricks-connect~=17.2.0", + ConstraintDeps: []string{"pyarrow~=21.0.0"}, + } + assert.Equal(t, []string{ + WarnRequiresPythonOverridden, + WarnDBConnectPinOverridden, + WarnUserConstraintConflict, + }, codes(detectWarnings(user, c))) +} + +func TestDetectMergeWarningsIncludeGroupIsDuplicatedNotOverridden(t *testing.T) { + // MergeManaged only rewrites a pin sitting in dev's own array. A pin reached + // through an include-group is left alone and the env's pin is inserted alongside + // it, so the merged file carries two pins for one package and uv cannot resolve. + // That is a different condition from an override and needs its own code — + // reporting "is replaced" here would state something untrue and hide a hard + // resolution failure behind a reassuring advisory. + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect==17.0.0"} + + indirect := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "spark"}] +spark = ["databricks-connect==16.1.0"] +`) + got := detectWarnings(indirect, c) + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(got)) + // The message must not claim a replacement that did not happen. + assert.NotContains(t, got[0].Message, "is replaced") + + // A pin directly in dev *is* rewritten in place, so that stays an override. + direct := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect==16.1.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinOverridden}, codes(detectWarnings(direct, c))) + + // With pins both in dev and behind an include-group, the merge rewrites dev's and + // leaves the included one, so both conditions hold and both are reported. Only + // flagging the override would go silent on the pin that still breaks resolution. + both := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect==16.1.0", {include-group = "spark"}] +spark = ["databricks-connect==15.0.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinOverridden, WarnDBConnectPinDuplicated}, + codes(detectWarnings(both, c))) + + // The merge is idempotent, but the included pin it does not rewrite is not fixed + // by re-running: the duplicate warning must persist for as long as the two pins do, + // or the user loses the only signal about a project uv cannot resolve. + merged, _, err := MergeManaged(both, c) + require.NoError(t, err) + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(detectWarnings(merged, c))) + + // An include-group cycle must terminate rather than recurse forever, and the pin + // behind it is still found. + cyclic := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "a"}] +a = [{include-group = "dev"}, "databricks-connect==16.1.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(detectWarnings(cyclic, c))) + + // PEP 735 normalizes group names the same way PEP 503 normalizes package names. + renamed := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "My_Spark.Group"}] +"my-spark-group" = ["databricks-connect==16.1.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(detectWarnings(renamed, c))) + + // An included pin that already matches the env needs no reconciliation. + matching := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "spark"}] +spark = ["databricks-connect==17.0.0"] +`) + assert.Empty(t, detectWarnings(matching, c)) + + // A dangling reference is not an error; there is simply no pin to compare. + dangling := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "absent"}] +`) + assert.Empty(t, detectWarnings(dangling, c)) +} + +func TestDetectMergeWarningsConflictInDependencyGroup(t *testing.T) { + // uv applies constraint-dependencies to the whole resolution, so a pin in the dev + // group breaks `uv sync` exactly like one in [project].dependencies. The dev group + // is where this command's audience keeps its pins, so it must be scanned too. + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["pyarrow==17.0.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow<19"}} + assert.Empty(t, detectWarnings(user, c), "17.0.0 satisfies <19 — no conflict") + + conflicting := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["pyarrow==21.0.0"] +`) + assert.Equal(t, []string{WarnUserConstraintConflict}, + codes(detectWarnings(conflicting, c)), "21.0.0 is outside <19") + + // A pin behind an include-group is part of the resolution too. + included := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "data"}] +data = ["pyarrow==21.0.0"] +`) + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(detectWarnings(included, c))) +} + +func TestDetectMergeWarningsSurvivesUnrelatedTOMLShapes(t *testing.T) { + // BurntSushi reports a per-key type mismatch as a whole-document error. Each shape + // below is legal in a real pyproject.toml but does not fit a stricter struct, and + // none of them is read by the requires-python check — so dropping every warning on + // one of them would silently hide an override the merge does perform. + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=18.0.0"} + for name, body := range map[string]string{ + // PDM/mkdocs style: a dependency group declared as a sub-table. + "group sub-table": `[project] +requires-python = ">=3.9" + +[dependency-groups] +dev = ["databricks-connect~=16.1.0"] + +[dependency-groups.docs] +mkdocs = "*" +`, + // PEP 621 permits a dependency to be a table in some tool dialects. + "table dependency": `[project] +requires-python = ">=3.9" +dependencies = ["pyarrow==21.0.0", {name = "x"}] + +[dependency-groups] +dev = ["databricks-connect~=16.1.0"] +`, + "group is a string": `[project] +requires-python = ">=3.9" + +[dependency-groups] +dev = "oops" +`, + } { + got := codes(detectWarnings([]byte(body), c)) + assert.Contains(t, got, WarnRequiresPythonOverridden, "%s: requires-python override must survive", name) + } + + // A genuine syntax error still yields nothing: there are no fields to compare. + assert.Empty(t, detectWarnings([]byte("this is : not valid toml ["), c)) +} + +func TestDetectMergeWarningsScansEveryGroupUVLocks(t *testing.T) { + // uv applies constraint-dependencies to the whole resolution and locks every + // declared group, so a conflicting pin outside dev fails `uv sync` with + // "requirements are unsatisfiable" just as a [project] one does. Scanning only dev + // would stay silent on exactly that failure. + c := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow<19"}} + + nonDevGroup := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +qa = ["pyarrow==21.0.0"] +`) + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(detectWarnings(nonDevGroup, c)), + "a pin in a group uv locks conflicts even though the group is not dev") + + optionalExtra := []byte(`[project] +requires-python = "==3.12.*" + +[project.optional-dependencies] +extra = ["pyarrow==21.0.0"] +`) + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(detectWarnings(optionalExtra, c)), + "uv resolves extras alongside the base dependencies") +} + +func TestDetectMergeWarningsReportsOneConflictPerRequirement(t *testing.T) { + // The same pin may be declared in several places uv locks. It is one conflict to + // fix, and repeating it would inflate the histogram consumers build from the codes. + user := []byte(`[project] +requires-python = "==3.12.*" +dependencies = ["pyarrow==21.0.0"] + +[dependency-groups] +dev = ["pyarrow==21.0.0", {include-group = "g"}] +g = ["pyarrow==21.0.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow<19"}} + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(detectWarnings(user, c))) + + // Two genuinely different conflicting pins for one package are each reported. + two := []byte(`[project] +requires-python = "==3.12.*" +dependencies = ["pyarrow==21.0.0"] + +[dependency-groups] +dev = ["pyarrow==22.0.0"] +`) + assert.Equal(t, []string{WarnUserConstraintConflict, WarnUserConstraintConflict}, + codes(detectWarnings(two, c))) +} + +func TestDetectMergeWarningsIsDeterministic(t *testing.T) { + // Group names are read from a Go map. Two keys that normalize to the same PEP 735 + // group, or several groups holding conflicting pins, must not let map iteration + // order decide which warning is reported. + c := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow<19", "numpy<2"}} + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["pyarrow==21.0.0"] +Dev = ["numpy==9.9.9"] +`) + want := codes(detectWarnings(user, c)) + assert.Len(t, want, 2, "both groups are locked by uv, so both conflicts are reported") + for range 200 { + assert.Equal(t, want, codes(detectWarnings(user, c))) + } +} + +func TestDetectMergeWarningsNonLiteralDevGroupIsNotAnOverride(t *testing.T) { + // MergeManaged finds the dev array with devKeyRe (`^\s*dev\s*=`), so a group spelled + // "Dev" is never rewritten: the merge adds its own dev key and the user's pin stays. + // Claiming an override here would describe a replacement that did not happen, and + // staying silent would hide a file uv rejects outright. + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect==17.0.0"} + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +Dev = ["databricks-connect==16.1.0"] +`) + got := detectWarnings(user, c) + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(got)) + assert.NotContains(t, got[0].Message, "is replaced") + + merged, _, err := MergeManaged(user, c) + require.NoError(t, err) + assert.Contains(t, string(merged), `"databricks-connect==16.1.0"`, "the user's pin is retained, not replaced") +} + +func TestDBConnectOverrideFollowsWhatTheMergeRewrites(t *testing.T) { + // The override warning must be driven by the merge's own answer, not by a second + // implementation of "is this pin in the dev array". MergeManaged rewrites only + // double-quoted elements, so each spelling below is left in place beside the managed + // pin — reporting it as replaced would be a false claim about the user's file. + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect==17.0.0"} + for name, body := range map[string]string{ + // A TOML literal string is a string, but not one replaceDbconnectElement matches. + "single-quoted element": `[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ['databricks-connect==16.1.0'] +`, + // A dotted key defines the same table but no line matches devKeyRe. + "top-level dotted key": `dependency-groups.dev = ["databricks-connect==16.1.0"] + +[project] +requires-python = "==3.12.*" +`, + // devKeyRe is literal, so a normalization-equal key is a different array. + "capitalized group": `[project] +requires-python = "==3.12.*" + +[dependency-groups] +Dev = ["databricks-connect==16.1.0"] +`, + } { + got := detectWarnings([]byte(body), c) + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(got), name) + assert.NotContains(t, got[0].Message, "is replaced", name) + + // The merged file really does carry both pins, which is what the code reports. + merged, _, err := MergeManaged([]byte(body), c) + require.NoError(t, err, name) + assert.Contains(t, string(merged), "databricks-connect==16.1.0", name) + assert.Contains(t, string(merged), "databricks-connect==17.0.0", name) + } +} + +func TestDBConnectDuplicateOnlyWhenRangesCannotBothHold(t *testing.T) { + // Two pins for one package are only a problem when nothing satisfies both. uv + // resolves an overlapping pair without complaint, so there is nothing to reconcile + // and no warning to give. + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=17.2.0"} + + overlapping := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "spark"}] +spark = ["databricks-connect>=16"] +`) + assert.Empty(t, detectWarnings(overlapping, c), ">=16 and ~=17.2.0 both hold at 17.2.x") + + disjoint := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "spark"}] +spark = ["databricks-connect==15.0.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(detectWarnings(disjoint, c)), + "==15.0.0 is outside ~=17.2.0, so uv cannot resolve") +} + +func TestDBConnectWarningsSecondDirectPinIsRetained(t *testing.T) { + // MergeManaged replaces only the first databricks-connect element in the dev array, + // so a second one survives and leaves two pins uv cannot resolve. + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect==17.0.0"} + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect==17.0.0", "databricks-connect==16.0.0"] +`) + // The first pin already matches the env, so there is nothing to override — but the + // second still has to be reconciled by hand. + assert.Equal(t, []string{WarnDBConnectPinDuplicated}, codes(detectWarnings(user, c))) +} + +func TestConstraintConflictsDuplicateEnvEntriesAreOrderIndependent(t *testing.T) { + // Constraint entries for one package compose as a conjunction, so they are + // intersected rather than letting one entry win. 19.0.0 is outside [20, 21) + // whichever order the artifact lists the bounds in. + user := []string{"pyarrow==19.0.0"} + assert.Equal(t, []string{WarnUserConstraintConflict}, + codes(constraintConflicts(user, []string{"pyarrow<21", "pyarrow>=20"}))) + assert.Equal(t, []string{WarnUserConstraintConflict}, + codes(constraintConflicts(user, []string{"pyarrow>=20", "pyarrow<21"}))) + + // A pin inside the intersection is not a conflict, again either way round. + inside := []string{"pyarrow==20.5.0"} + assert.Empty(t, constraintConflicts(inside, []string{"pyarrow<21", "pyarrow>=20"})) + assert.Empty(t, constraintConflicts(inside, []string{"pyarrow>=20", "pyarrow<21"})) +} + +func TestSplitDepSpecExtrasAndMarkers(t *testing.T) { + // Extras never narrow the version range, so they are stripped and the pin + // underneath is still compared. + name, spec, ok := splitDepSpec("pyarrow[compute,parquet]==17.0.0") + assert.True(t, ok) + assert.Equal(t, "pyarrow", name) + assert.Equal(t, "==17.0.0", spec) + + // A marker makes the requirement conditional on the resolving interpreter, + // which we do not evaluate — so it is skipped rather than compared, with or + // without extras in front of it. + _, _, ok = splitDepSpec(`pyarrow==17.0.0; python_version < "3.12"`) + assert.False(t, ok) + _, _, ok = splitDepSpec(`pyarrow[compute]==17.0.0; python_version < "3.12"`) + assert.False(t, ok) + + // PEP 503 normalization applies to the name on both sides of the lookup. + name, spec, ok = splitDepSpec("PyArrow_Extra ==1.0") + assert.True(t, ok) + assert.Equal(t, "pyarrow-extra", name) + assert.Equal(t, "==1.0", spec) +} + +func TestDetectMergeWarningsConflictThroughExtras(t *testing.T) { + user := []byte(`[project] +dependencies = ["pyarrow[compute]==17.0.0"] +`) + c := Constraints{ConstraintDeps: []string{"pyarrow~=21.0.0"}} + got := detectWarnings(user, c) + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(got)) +} + +func TestRangesDisjoint(t *testing.T) { + cases := []struct { + user, env string + disjoint bool + why string + }{ + {"==17.0.0", "~=21.0.0", true, "exact pin below the compatible range"}, + {"==21.0.3", "~=21.0.0", false, "exact pin inside ~=21.0.0 == >=21.0.0,==21.0.*"}, + {"==21.5.0", "~=21.0.0", true, "~=21.0.0 fixes 21.0.*, so 21.5.0 is outside it"}, + {"==22.0.0", "~=21.0.0", true, "exact pin above the compatible upper bound"}, + {"==1.0", "==2.0", true, "two different exact pins"}, + {"==2.0", "==2.0", false, "identical exact pins"}, + {"==2", "==2.0.0", false, "trailing zeros denote the same release"}, + {">=2.0", "==2.5", false, "2.5 is above the floor — overlapping"}, + {"!=2.0", "==2.0", false, "!= is not modeled — never a conflict"}, + {">=2.0,<3.0", "==5.0", true, "5.0 is above the compound range's ceiling"}, + {"==2.*", "==2.0", false, "wildcards are unparsed — no conflict"}, + {"", "~=21.0.0", false, "no user spec — nothing to compare"}, + + // Opposite-direction bounds. This is the shape the published constraint + // artifacts use (pyarrow<19, pandas<3), so failing to decide it would make the + // warning miss the common case and under-report the metric silently. + {">=20", "<19", true, "floor above the ceiling"}, + {">=4", "<3", true, "floor above the ceiling, single segment"}, + {">=19", "<20", false, "ranges overlap on [19, 20)"}, + {">2.0", "<2.0", true, "strict bounds meeting at one excluded point"}, + {">=2.0", "<2.0", true, "inclusive floor at an exclusive ceiling"}, + {">=2.0", "<=2.0", false, "both include the single shared version 2.0"}, + {">2.0", "<=2.0", true, "the shared endpoint is excluded by the floor"}, + {"<19", "<3", false, "same direction always shares the low end"}, + {">=20", ">=3", false, "same direction always shares the high end"}, + + // ~= against a bound or another ~=. The ceiling depends on how many segments + // the base has: ~=21.0 expands to ">=21.0, ==21.*" -> [21.0, 22.0), whereas + // ~=21.0.0 expands to ">=21.0.0, ==21.0.*" -> [21.0.0, 21.1.0). + {"~=17.0", "~=21.0", true, "[17.0, 18.0) is entirely below [21.0, 22.0)"}, + {"~=21.0", "~=21.1", false, "[21.1, 22.0) is a subset of [21.0, 22.0)"}, + {"~=21.0.0", "~=21.1.0", true, "[21.0.0, 21.1.0) ends where [21.1.0, 21.2.0) begins"}, + {"~=21.0", "~=21.0", false, "identical compatible ranges"}, + {"~=17.0", "<19", false, "[17.0, 18.0) lies below the ceiling 19"}, + {"~=21.0", "<19", true, "floor 21.0 is above the ceiling 19"}, + {"~=21.0", ">=22", true, "ceiling 22.0 is exclusive, so it meets the floor 22"}, + {"~=21.0", ">=21.5", false, "21.5 is inside [21.0, 22.0)"}, + // PEP 440 requires two release segments after "~=", so a single-segment base + // has no defined range. It must stay undecidable rather than being read as a + // proof of disjointness: 2.0 and 2.31.0 both plainly satisfy any reading of ~=2. + {"~=2", "==2.0", false, "single-segment ~= is malformed — undecidable, not disjoint"}, + {"==2.0", "~=2", false, "same, with the malformed base on the env side"}, + {"~=2", "==2.31.0", false, "single-segment ~= never decides a conflict"}, + {"~=2.0", "==2.31.0", false, "valid ~=2.0 admits 2.31.0 (>=2.0, ==2.*)"}, + {"~=2.1", "==2.0.5", true, "valid ~=2.1 excludes 2.0.5 (below the floor)"}, + } + for _, tc := range cases { + assert.Equalf(t, tc.disjoint, rangesDisjoint(tc.user, tc.env), + "rangesDisjoint(%q,%q): %s", tc.user, tc.env, tc.why) + } +}