From 72185baf70ff8d93070d6aeeb35212e8545b2a3d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 23 Oct 2024 10:57:13 -0400 Subject: [PATCH] Avoid rewriting `[[tool.uv.index]]` entries when credentials are provided (#8502) ## Summary Instead of creating a new entry, we should reuse the existing entry (to preserve decor); similarly, we should avoid overwriting fields that are already "correct". Closes https://github.com/astral-sh/uv/issues/8483. --- Cargo.lock | 1 + crates/uv-workspace/Cargo.toml | 1 + crates/uv-workspace/src/pyproject_mut.rs | 180 ++++++++++++++----- crates/uv/tests/it/edit.rs | 214 +++++++++++++++++++++-- 4 files changed, 341 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c4dfdd183..d3938db14 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5341,6 +5341,7 @@ dependencies = [ "toml_edit", "tracing", "url", + "uv-cache-key", "uv-distribution-types", "uv-fs", "uv-git", diff --git a/crates/uv-workspace/Cargo.toml b/crates/uv-workspace/Cargo.toml index 967a90ae3..d77d08fcd 100644 --- a/crates/uv-workspace/Cargo.toml +++ b/crates/uv-workspace/Cargo.toml @@ -16,6 +16,7 @@ doctest = false workspace = true [dependencies] +uv-cache-key = { workspace = true } uv-distribution-types = { workspace = true } uv-fs = { workspace = true, features = ["tokio", "schemars"] } uv-git = { workspace = true } diff --git a/crates/uv-workspace/src/pyproject_mut.rs b/crates/uv-workspace/src/pyproject_mut.rs index f24c960cf..7f5b3e9cf 100644 --- a/crates/uv-workspace/src/pyproject_mut.rs +++ b/crates/uv-workspace/src/pyproject_mut.rs @@ -1,9 +1,15 @@ -use itertools::Itertools; use std::path::Path; use std::str::FromStr; use std::{fmt, mem}; + +use itertools::Itertools; use thiserror::Error; -use toml_edit::{Array, ArrayOfTables, DocumentMut, Item, RawString, Table, TomlError, Value}; +use toml_edit::{ + Array, ArrayOfTables, DocumentMut, Formatted, Item, RawString, Table, TomlError, Value, +}; +use url::Url; + +use uv_cache_key::CanonicalUrl; use uv_distribution_types::Index; use uv_fs::PortablePath; use uv_pep440::{Version, VersionSpecifier, VersionSpecifiers}; @@ -220,70 +226,160 @@ impl PyProjectTomlMut { .ok_or(Error::MalformedSources)? .entry("index") .or_insert(Item::ArrayOfTables(ArrayOfTables::new())) - .as_array_of_tables() + .as_array_of_tables_mut() .ok_or(Error::MalformedSources)?; - let mut table = Table::new(); - if let Some(name) = index.name.as_ref() { - table.insert("name", toml_edit::value(name.to_string())); - } else if let Some(name) = existing + // If there's already an index with the same name or URL, update it (and move it to the top). + let mut table = existing .iter() .find(|table| { - table + // If the index has the same name, reuse it. + if let Some(index) = index.name.as_deref() { + if table + .get("name") + .and_then(|name| name.as_str()) + .is_some_and(|name| name == index) + { + return true; + } + } + + // If the index is the default, and there's another default index, reuse it. + if index.default + && table + .get("default") + .is_some_and(|default| default.as_bool() == Some(true)) + { + return true; + } + + // If there's another index with the same URL, reuse it. + if table .get("url") - .is_some_and(|url| url.as_str() == Some(index.url.url().as_str())) + .and_then(|item| item.as_str()) + .and_then(|url| Url::parse(url).ok()) + .is_some_and(|url| { + CanonicalUrl::new(&url) == CanonicalUrl::new(index.url.url()) + }) + { + return true; + } + + false }) - .and_then(|existing| existing.get("name")) - { - // If there's an existing index with the same URL, and a name, preserve the name. - table.insert("name", name.clone()); - } - table.insert("url", toml_edit::value(index.url.to_string())); - if index.default { - table.insert("default", toml_edit::value(true)); + .cloned() + .unwrap_or_default(); + + // If necessary, update the name. + if let Some(index) = index.name.as_deref() { + if !table + .get("name") + .and_then(|name| name.as_str()) + .is_some_and(|name| name == index) + { + let mut formatted = Formatted::new(index.to_string()); + if let Some(value) = table.get("name").and_then(Item::as_value) { + if let Some(prefix) = value.decor().prefix() { + formatted.decor_mut().set_prefix(prefix.clone()); + } + if let Some(suffix) = value.decor().suffix() { + formatted.decor_mut().set_suffix(suffix.clone()); + } + } + table.insert("name", Value::String(formatted).into()); + } } - // Push the item to the table. - let mut updated = ArrayOfTables::new(); - updated.push(table); - for table in existing { - // If there's another index with the same name, replace it. - if table - .get("name") - .is_some_and(|name| name.as_str() == index.name.as_deref()) + // If necessary, update the URL. + if !table + .get("url") + .and_then(|item| item.as_str()) + .and_then(|url| Url::parse(url).ok()) + .is_some_and(|url| CanonicalUrl::new(&url) == CanonicalUrl::new(index.url.url())) + { + let mut formatted = Formatted::new(index.url.to_string()); + if let Some(value) = table.get("url").and_then(Item::as_value) { + if let Some(prefix) = value.decor().prefix() { + formatted.decor_mut().set_prefix(prefix.clone()); + } + if let Some(suffix) = value.decor().suffix() { + formatted.decor_mut().set_suffix(suffix.clone()); + } + } + table.insert("url", Value::String(formatted).into()); + } + + // If necessary, update the default. + if index.default { + if !table + .get("default") + .and_then(toml_edit::Item::as_bool) + .is_some_and(|default| default) { - continue; + let mut formatted = Formatted::new(true); + if let Some(value) = table.get("default").and_then(Item::as_value) { + if let Some(prefix) = value.decor().prefix() { + formatted.decor_mut().set_prefix(prefix.clone()); + } + if let Some(suffix) = value.decor().suffix() { + formatted.decor_mut().set_suffix(suffix.clone()); + } + } + table.insert("default", Value::Boolean(formatted).into()); + } + } + + // Remove any replaced tables. + existing.retain(|table| { + // If the index has the same name, skip it. + if let Some(index) = index.name.as_deref() { + if table + .get("name") + .and_then(|name| name.as_str()) + .is_some_and(|name| name == index) + { + return false; + } } - // If there's another default index, remove it. + // If there's another default index, skip it. if index.default && table .get("default") .is_some_and(|default| default.as_bool() == Some(true)) { - continue; + return false; } - // If there's another index with the same URL, replace it. + // If there's another index with the same URL, skip it. if table .get("url") - .is_some_and(|url| url.as_str() == Some(index.url.url().as_str())) + .and_then(|item| item.as_str()) + .and_then(|url| Url::parse(url).ok()) + .is_some_and(|url| CanonicalUrl::new(&url) == CanonicalUrl::new(index.url.url())) { - continue; + return false; } - updated.push(table.clone()); + true + }); + + // Set the position to the minimum, if it's not already the first element. + if let Some(min) = existing.iter().filter_map(toml_edit::Table::position).min() { + // if !table.position().is_some_and(|position| position < min) { + table.set_position(min); + + // Increment the position of all existing elements. + for table in existing.iter_mut() { + if let Some(position) = table.position() { + table.set_position(position + 1); + } + } + // } } - self.doc - .entry("tool") - .or_insert(implicit()) - .as_table_mut() - .ok_or(Error::MalformedSources)? - .entry("uv") - .or_insert(implicit()) - .as_table_mut() - .ok_or(Error::MalformedSources)? - .insert("index", Item::ArrayOfTables(updated)); + + // Push the item to the table. + existing.push(table); Ok(()) } diff --git a/crates/uv/tests/it/edit.rs b/crates/uv/tests/it/edit.rs index 9c5727715..01d9a1059 100644 --- a/crates/uv/tests/it/edit.rs +++ b/crates/uv/tests/it/edit.rs @@ -5507,7 +5507,7 @@ fn add_index() -> Result<()> { ); }); - // Adding a subsequent index should put it _below_ the existing index. + // Adding a subsequent index should put it _above_ the existing index. uv_snapshot!(context.filters(), context.add().arg("jinja2").arg("--index").arg("pytorch=https://download.pytorch.org/whl/cu121").env_remove(EnvVars::UV_EXCLUDE_NEWER), @r###" success: true exit_code: 0 @@ -5541,11 +5541,11 @@ fn add_index() -> Result<()> { name = "pytorch" url = "https://download.pytorch.org/whl/cu121" - [[tool.uv.index]] - url = "https://pypi.org/simple" - [tool.uv.sources] jinja2 = { index = "pytorch" } + + [[tool.uv.index]] + url = "https://pypi.org/simple" "### ); }); @@ -5638,15 +5638,15 @@ fn add_index() -> Result<()> { "jinja2>=3.1.3", ] + [tool.uv.sources] + jinja2 = { index = "pytorch" } + [[tool.uv.index]] name = "pytorch" url = "https://test.pypi.org/simple" [[tool.uv.index]] url = "https://pypi.org/simple" - - [tool.uv.sources] - jinja2 = { index = "pytorch" } "### ); }); @@ -5748,15 +5748,15 @@ fn add_index() -> Result<()> { "typing-extensions>=4.12.2", ] + [tool.uv.sources] + jinja2 = { index = "pytorch" } + [[tool.uv.index]] url = "https://pypi.org/simple" [[tool.uv.index]] name = "pytorch" url = "https://test.pypi.org/simple" - - [tool.uv.sources] - jinja2 = { index = "pytorch" } "### ); }); @@ -5867,15 +5867,15 @@ fn add_index() -> Result<()> { "typing-extensions>=4.12.2", ] + [tool.uv.sources] + jinja2 = { index = "pytorch" } + [[tool.uv.index]] name = "pytorch" url = "https://test.pypi.org/simple" [[tool.uv.index]] url = "https://pypi.org/simple" - - [tool.uv.sources] - jinja2 = { index = "pytorch" } "### ); }); @@ -6133,6 +6133,194 @@ fn add_default_index_url() -> Result<()> { Ok(()) } +#[test] +fn add_index_credentials() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = [] + + # Set an internal index as the default, without credentials. + [[tool.uv.index]] + name = "internal" + url = "https://pypi-proxy.fly.dev/basic-auth/simple" + default = true + "#})?; + + // Provide credentials for the index via the environment variable. + uv_snapshot!(context.filters(), context.add().arg("iniconfig==2.0.0").env("UV_DEFAULT_INDEX", "https://public:heron@pypi-proxy.fly.dev/basic-auth/simple"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 2 packages in [TIME] + Prepared 1 package in [TIME] + Installed 1 package in [TIME] + + iniconfig==2.0.0 + "###); + + let pyproject_toml = fs_err::read_to_string(context.temp_dir.join("pyproject.toml"))?; + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + pyproject_toml, @r###" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = [ + "iniconfig==2.0.0", + ] + + # Set an internal index as the default, without credentials. + [[tool.uv.index]] + name = "internal" + url = "https://pypi-proxy.fly.dev/basic-auth/simple" + default = true + "### + ); + }); + + let lock = fs_err::read_to_string(context.temp_dir.join("uv.lock"))?; + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + lock, @r###" + version = 1 + requires-python = ">=3.12" + + [options] + exclude-newer = "2024-03-25T00:00:00Z" + + [[package]] + name = "iniconfig" + version = "2.0.0" + source = { registry = "https://pypi-proxy.fly.dev/basic-auth/simple" } + sdist = { url = "https://pypi-proxy.fly.dev/basic-auth/files/packages/d7/4b/cbd8e699e64a6f16ca3a8220661b5f83792b3017d0f79807cb8708d33913/iniconfig-2.0.0.tar.gz", hash = "sha256:2d91e135bf72d31a410b17c16da610a82cb55f6b0477d1a902134b24a455b8b3", size = 4646 } + wheels = [ + { url = "https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl", hash = "sha256:b6a85871a79d2e3b22d2d1b94ac2824226a63c6b741c88f7ae975f18b6778374", size = 5892 }, + ] + + [[package]] + name = "project" + version = "0.1.0" + source = { virtual = "." } + dependencies = [ + { name = "iniconfig" }, + ] + + [package.metadata] + requires-dist = [{ name = "iniconfig", specifier = "==2.0.0" }] + "### + ); + }); + + Ok(()) +} + +#[test] +fn add_index_comments() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = [] + + [[tool.uv.index]] + name = "internal" + url = "https://test.pypi.org/simple" # This is a test index. + default = true + "#})?; + + // Preserve the comment on the index URL, despite replacing it. + uv_snapshot!(context.filters(), context.add().arg("iniconfig==2.0.0").env("UV_DEFAULT_INDEX", "https://pypi.org/simple"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 2 packages in [TIME] + Prepared 1 package in [TIME] + Installed 1 package in [TIME] + + iniconfig==2.0.0 + "###); + + let pyproject_toml = fs_err::read_to_string(context.temp_dir.join("pyproject.toml"))?; + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + pyproject_toml, @r###" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = [ + "iniconfig==2.0.0", + ] + + [[tool.uv.index]] + name = "internal" + url = "https://pypi.org/simple" # This is a test index. + default = true + "### + ); + }); + + let lock = fs_err::read_to_string(context.temp_dir.join("uv.lock"))?; + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + lock, @r###" + version = 1 + requires-python = ">=3.12" + + [options] + exclude-newer = "2024-03-25T00:00:00Z" + + [[package]] + name = "iniconfig" + version = "2.0.0" + source = { registry = "https://pypi.org/simple" } + sdist = { url = "https://files.pythonhosted.org/packages/d7/4b/cbd8e699e64a6f16ca3a8220661b5f83792b3017d0f79807cb8708d33913/iniconfig-2.0.0.tar.gz", hash = "sha256:2d91e135bf72d31a410b17c16da610a82cb55f6b0477d1a902134b24a455b8b3", size = 4646 } + wheels = [ + { url = "https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl", hash = "sha256:b6a85871a79d2e3b22d2d1b94ac2824226a63c6b741c88f7ae975f18b6778374", size = 5892 }, + ] + + [[package]] + name = "project" + version = "0.1.0" + source = { virtual = "." } + dependencies = [ + { name = "iniconfig" }, + ] + + [package.metadata] + requires-dist = [{ name = "iniconfig", specifier = "==2.0.0" }] + "### + ); + }); + + Ok(()) +} + /// Accidentally add a dependency on the project itself. #[test] fn add_self() -> Result<()> {