fix(config): support repository names containing dots#10717
fix(config): support repository names containing dots#10717LouisLau-art wants to merge 4 commits intopython-poetry:mainfrom
Conversation
Reviewer's GuideImplements escaped-dot support in config key parsing and consistently escapes repository names with dots when reading and writing related configuration (repositories, http-basic, pypi-token, certificates), updating core config sources, consumers, and tests accordingly. Sequence diagram for setting HTTP password with dotted repository namesequenceDiagram
actor User
participant PasswordManager
participant Config
participant AuthConfigSource
participant ConfigKeyUtils
User->>PasswordManager: set_http_password(repo_name "my.repo", username, password)
PasswordManager->>Config: get(use_keyring)
alt use_keyring is false
PasswordManager->>PasswordManager: warn_plaintext_credentials_stored()
else use_keyring is true
PasswordManager->>Keyring: set_password(repo_name, username, password)
end
PasswordManager->>ConfigKeyUtils: escape_config_key(repo_name)
ConfigKeyUtils-->>PasswordManager: repository "my\.repo"
PasswordManager->>AuthConfigSource: add_property("http-basic." + repository, auth)
activate AuthConfigSource
AuthConfigSource->>ConfigKeyUtils: split_config_key("http-basic.my\.repo")
ConfigKeyUtils-->>AuthConfigSource: ["http-basic", "my.repo"]
AuthConfigSource-->>PasswordManager: property stored in nested config
deactivate AuthConfigSource
User-->>User: HTTP password saved for dotted repository name
Class diagram for config key utilities and their usageclassDiagram
class ConfigKeyUtils {
+list~str~ split_config_key(key str)
+str escape_config_key(key str)
}
class ConfigSource {
<<abstract>>
+get_property(key str) Any
+add_property(key str, value Any) void
+remove_property(key str) void
}
class DictConfigSource {
-config dict~str, Any~
+get_property(key str) Any
+add_property(key str, value Any) void
+remove_property(key str) void
}
class FileConfigSource {
-file TOMLFile
+get_property(key str) Any
+add_property(key str, value Any) void
+remove_property(key str) void
}
class Config {
+get(setting_name str, default Any) Any
}
class PasswordManager {
-use_keyring bool
-_config Config
+set_pypi_token(repo_name str, token str) void
+get_pypi_token(repo_name str) str
+delete_pypi_token(repo_name str) void
+get_http_auth(repo_name str) HTTPAuthCredential
+set_http_password(repo_name str, username str, password str) void
+delete_http_password(repo_name str) void
}
class ConfigCommand {
+handle() int
}
class Authenticator {
+configured_repositories() dict~str, AuthenticatorRepositoryConfig~
}
class RepositoryCertificateConfig {
+create(repository str, config Config) RepositoryCertificateConfig
}
class Publisher {
-_poetry Poetry
+publish(repository_name str) void
}
ConfigSource <|-- DictConfigSource
ConfigSource <|-- FileConfigSource
DictConfigSource ..> ConfigKeyUtils : uses split_config_key
FileConfigSource ..> ConfigKeyUtils : uses split_config_key
Config ..> ConfigKeyUtils : uses split_config_key
PasswordManager ..> ConfigKeyUtils : uses escape_config_key
ConfigCommand ..> ConfigKeyUtils : uses escape_config_key
Authenticator ..> ConfigKeyUtils : uses escape_config_key
RepositoryCertificateConfig ..> ConfigKeyUtils : uses escape_config_key
Publisher ..> ConfigKeyUtils : uses escape_config_key
PasswordManager --> Config : uses
Config --> ConfigSource : auth_config_source
ConfigCommand --> Config : uses
Authenticator --> Config : uses
RepositoryCertificateConfig --> Config : uses
Publisher --> Config : uses
Flow diagram for split_config_key behavior with escaped dotsflowchart TD
A["Start split_config_key(key)"] --> B[Initialize parts as empty list]
B --> C[Initialize current as empty list]
C --> D[Set escaped to False]
D --> E{For each char in key}
E --> F{escaped is True?}
F -->|Yes| G[Append char to current]
G --> H[Set escaped to False]
H --> E
F -->|No| I{char is backslash?}
I -->|Yes| J[Set escaped to True]
J --> E
I -->|No| K{char is dot?}
K -->|Yes| L[Append joined current to parts]
L --> M[Reset current to empty]
M --> E
K -->|No| N[Append char to current]
N --> E
E -->|Loop finished| O{escaped is True?}
O -->|Yes| P[Append backslash to current]
P --> Q[Append joined current to parts]
O -->|No| Q[Append joined current to parts]
Q --> R[Return parts]
R --> S[End]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/utils/test_password_manager.py:135-143` </location>
<code_context>
assert auth["password"] == "baz"
+def test_set_http_password_with_dot_in_repo_name(
+ config: Config, with_fail_keyring: None
+) -> None:
+ manager = PasswordManager(config)
+ manager.set_http_password("foo.bar", "baz", "qux")
+
+ auth = config.get("http-basic.foo\\.bar")
+ assert auth["username"] == "baz"
+ assert auth["password"] == "qux"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for get/delete flows and keyring-enabled mode with dotted repository names in HTTP auth tests
To also cover the new `PasswordManager` behavior, please add tests that:
- Call `get_http_auth("foo.bar")` after setting credentials and assert the correct values are returned, both with and without keyring usage.
- Call `delete_http_password("foo.bar")` and assert the escaped key is removed, and when keyring is enabled, that the keyring delete path is taken without changing the stored config.
This will cover the full read/write/delete flow for dotted repository names in HTTP basic auth.
Suggested implementation:
```python
def test_set_http_password_with_dot_in_repo_name(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
auth = config.get("http-basic.foo\\.bar")
assert auth["username"] == "baz"
assert auth["password"] == "qux"
def test_get_http_auth_with_dot_in_repo_name_config_only(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# get_http_auth must resolve the dotted repo name using the escaped key
auth = manager.get_http_auth("foo.bar")
# Depending on implementation this may be a mapping or an object with attributes.
# The assertions below assume a mapping interface for consistency with other tests.
assert auth is not None
assert auth["username"] == "baz"
assert auth["password"] == "qux"
def test_delete_http_password_with_dot_in_repo_name_config_only(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# Ensure it was written under the escaped key
auth = config.get("http-basic.foo\\.bar")
assert auth["username"] == "baz"
assert auth["password"] == "qux"
# Delete and verify the escaped key is removed from config
manager.delete_http_password("foo.bar")
deleted_auth = config.get("http-basic.foo\\.bar")
# Assuming Config.get() returns None when the section/option is missing
assert deleted_auth is None
def test_get_http_auth_with_dot_in_repo_name_keyring_enabled(
config: Config, with_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# When keyring is enabled, get_http_auth should still resolve dotted names correctly
auth = manager.get_http_auth("foo.bar")
assert auth is not None
assert auth["username"] == "baz"
assert auth["password"] == "qux"
def test_delete_http_password_with_dot_in_repo_name_keyring_enabled(
config: Config, with_keyring: None, keyring_backend
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# Ensure we have credentials before deletion
auth = manager.get_http_auth("foo.bar")
assert auth is not None
# Delete via PasswordManager; with keyring enabled this should invoke the keyring delete path
manager.delete_http_password("foo.bar")
# Config entry should remain unchanged when keyring is enabled
# (PasswordManager should rely on keyring deletion instead of mutating stored config)
remaining_auth = config.get("http-basic.foo\\.bar")
assert remaining_auth is not None
assert remaining_auth["username"] == "baz"
assert remaining_auth["password"] == "qux"
# And the keyring backend should have been asked to delete the credentials
# Exact assertion depends on how keyring_backend is exposed/mocked:
# below assumes a spy-like interface with .get_password/.delete_password
assert keyring_backend.delete_calls # at least one delete call was recorded
assert any("foo.bar" in call[0] for call in keyring_backend.delete_calls)
```
The above edits assume the following about the existing test and helper APIs:
1. `PasswordManager.get_http_auth(name)`:
- Returns a mapping-like object (e.g. `dict`) with `"username"` and `"password"` keys.
- If it instead returns an object with attributes (e.g. `.username`, `.password`), update the assertions accordingly.
2. `Config.get(key)`:
- Returns `None` when the key/section is missing.
- If it returns a section mapping (e.g. `config["http-basic"]["foo\\.bar"]` is the pattern used elsewhere), adjust the delete assertions to match the existing style, e.g.:
- Access the correct section and assert the escaped repo key is not present after delete.
3. Keyring fixtures:
- The tests above assume you already have fixtures named `with_keyring` and `keyring_backend` (or similar) used elsewhere in this file to test keyring-enabled behavior.
- `keyring_backend` is assumed to expose recorded delete calls in `keyring_backend.delete_calls` with some structure like `[(service, username), ...]`.
- If your current tests use a different spy/mocking pattern (e.g. `keyring_backend.delete_password.assert_called_with(...)` or `keyring_mock.calls`), replace the last two assertions in `test_delete_http_password_with_dot_in_repo_name_keyring_enabled` with the appropriate expectation to verify that the keyring delete path was invoked.
4. If, in keyring-enabled mode, `PasswordManager.set_http_password` does *not* persist HTTP basic auth into config at all (only into keyring), update the "config entry should remain unchanged" assertion to instead verify that config has no stored credentials and rely solely on the keyring assertions.
Please align the fixtures (`with_keyring`, `keyring_backend`) and the exact assertions with how keyring is currently tested elsewhere in `tests/utils/test_password_manager.py`.
</issue_to_address>
### Comment 2
<location> `tests/utils/test_password_manager.py:203-209` </location>
<code_context>
assert config.get("pypi-token.foo") == "baz"
+def test_set_pypi_token_with_dot_in_repo_name(
+ config: Config, with_fail_keyring: None
+) -> None:
+ manager = PasswordManager(config)
+ manager.set_pypi_token("foo.bar", "baz")
+
+ assert config.get("pypi-token.foo\\.bar") == "baz"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add complementary tests for get/delete pypi token and keyring mode with dotted repository names
To cover the new behavior more completely, please also add tests for:
- `get_pypi_token("foo.bar")` when the token is stored in config (no keyring), asserting it reads from the escaped key.
- `delete_pypi_token("foo.bar")`, asserting it removes the escaped key.
- A case where the keyring is available (without `with_fail_keyring`), confirming dotted repo names still work and the escaping logic doesn’t affect the keyring path.
These will exercise all code paths for dotted repository names.
Suggested implementation:
```python
def test_get_pypi_token_with_dot_in_repo_name_from_config(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
# Simulate an existing config entry using the escaped key
config["pypi-token.foo\\.bar"] = "baz"
token = manager.get_pypi_token("foo.bar")
assert token == "baz"
def test_delete_pypi_token_with_dot_in_repo_name_from_config(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
config["pypi-token.foo\\.bar"] = "baz"
manager.delete_pypi_token("foo.bar")
# Escaped key should be removed from config
assert "pypi-token.foo\\.bar" not in config
def test_pypi_token_with_dot_in_repo_name_with_keyring(
config: Config, monkeypatch: pytest.MonkeyPatch
) -> None:
manager = PasswordManager(config)
# Use a dummy in-memory keyring to observe key usage
stored: dict[tuple[str, str], str] = {}
class DummyKeyring:
def set_password(self, system: str, username: str, password: str) -> None:
stored[(system, username)] = password
def get_password(self, system: str, username: str) -> str | None:
return stored.get((system, username))
def delete_password(self, system: str, username: str) -> None:
stored.pop((system, username), None)
dummy = DummyKeyring()
# Patch the keyring used by PasswordManager so keyring is "available"
monkeypatch.setattr("poetry.utils.password_manager.keyring", dummy)
manager.set_pypi_token("foo.bar", "baz")
# Ensure the dotted repo name is used as-is in the keyring username
assert any(username == "foo.bar" for (_system, username) in stored.keys())
token = manager.get_pypi_token("foo.bar")
assert token == "baz"
manager.delete_pypi_token("foo.bar")
# After deletion, the key should be removed from the dummy keyring
assert not any(username == "foo.bar" for (_system, username) in stored.keys())
def test_set_http_password_with_dot_in_repo_name(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
auth = config.get("http-basic.foo\\.bar")
assert auth["username"] == "baz"
assert auth["password"] == "qux"
```
1. If `PasswordManager` imports `keyring` from a different module path than `"poetry.utils.password_manager"`, adjust the `monkeypatch.setattr` target accordingly so it patches the exact object used by `PasswordManager`.
2. If this test module does not yet import `pytest`, add `import pytest` at the top of the file (or extend an existing import) so `pytest.MonkeyPatch` and the `monkeypatch` fixture work.
3. If the project targets Python versions without support for `dict[tuple[str, str], str]` type hints, change the annotation to `Dict[Tuple[str, str], str]` and ensure `from typing import Dict, Tuple` is imported.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_set_http_password_with_dot_in_repo_name( | ||
| config: Config, with_fail_keyring: None | ||
| ) -> None: | ||
| manager = PasswordManager(config) | ||
| manager.set_http_password("foo.bar", "baz", "qux") | ||
|
|
||
| auth = config.get("http-basic.foo\\.bar") | ||
| assert auth["username"] == "baz" | ||
| assert auth["password"] == "qux" |
There was a problem hiding this comment.
suggestion (testing): Add coverage for get/delete flows and keyring-enabled mode with dotted repository names in HTTP auth tests
To also cover the new PasswordManager behavior, please add tests that:
- Call
get_http_auth("foo.bar")after setting credentials and assert the correct values are returned, both with and without keyring usage. - Call
delete_http_password("foo.bar")and assert the escaped key is removed, and when keyring is enabled, that the keyring delete path is taken without changing the stored config.
This will cover the full read/write/delete flow for dotted repository names in HTTP basic auth.
Suggested implementation:
def test_set_http_password_with_dot_in_repo_name(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
auth = config.get("http-basic.foo\\.bar")
assert auth["username"] == "baz"
assert auth["password"] == "qux"
def test_get_http_auth_with_dot_in_repo_name_config_only(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# get_http_auth must resolve the dotted repo name using the escaped key
auth = manager.get_http_auth("foo.bar")
# Depending on implementation this may be a mapping or an object with attributes.
# The assertions below assume a mapping interface for consistency with other tests.
assert auth is not None
assert auth["username"] == "baz"
assert auth["password"] == "qux"
def test_delete_http_password_with_dot_in_repo_name_config_only(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# Ensure it was written under the escaped key
auth = config.get("http-basic.foo\\.bar")
assert auth["username"] == "baz"
assert auth["password"] == "qux"
# Delete and verify the escaped key is removed from config
manager.delete_http_password("foo.bar")
deleted_auth = config.get("http-basic.foo\\.bar")
# Assuming Config.get() returns None when the section/option is missing
assert deleted_auth is None
def test_get_http_auth_with_dot_in_repo_name_keyring_enabled(
config: Config, with_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# When keyring is enabled, get_http_auth should still resolve dotted names correctly
auth = manager.get_http_auth("foo.bar")
assert auth is not None
assert auth["username"] == "baz"
assert auth["password"] == "qux"
def test_delete_http_password_with_dot_in_repo_name_keyring_enabled(
config: Config, with_keyring: None, keyring_backend
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
# Ensure we have credentials before deletion
auth = manager.get_http_auth("foo.bar")
assert auth is not None
# Delete via PasswordManager; with keyring enabled this should invoke the keyring delete path
manager.delete_http_password("foo.bar")
# Config entry should remain unchanged when keyring is enabled
# (PasswordManager should rely on keyring deletion instead of mutating stored config)
remaining_auth = config.get("http-basic.foo\\.bar")
assert remaining_auth is not None
assert remaining_auth["username"] == "baz"
assert remaining_auth["password"] == "qux"
# And the keyring backend should have been asked to delete the credentials
# Exact assertion depends on how keyring_backend is exposed/mocked:
# below assumes a spy-like interface with .get_password/.delete_password
assert keyring_backend.delete_calls # at least one delete call was recorded
assert any("foo.bar" in call[0] for call in keyring_backend.delete_calls)The above edits assume the following about the existing test and helper APIs:
-
PasswordManager.get_http_auth(name):- Returns a mapping-like object (e.g.
dict) with"username"and"password"keys. - If it instead returns an object with attributes (e.g.
.username,.password), update the assertions accordingly.
- Returns a mapping-like object (e.g.
-
Config.get(key):- Returns
Nonewhen the key/section is missing. - If it returns a section mapping (e.g.
config["http-basic"]["foo\\.bar"]is the pattern used elsewhere), adjust the delete assertions to match the existing style, e.g.:- Access the correct section and assert the escaped repo key is not present after delete.
- Returns
-
Keyring fixtures:
- The tests above assume you already have fixtures named
with_keyringandkeyring_backend(or similar) used elsewhere in this file to test keyring-enabled behavior. keyring_backendis assumed to expose recorded delete calls inkeyring_backend.delete_callswith some structure like[(service, username), ...].- If your current tests use a different spy/mocking pattern (e.g.
keyring_backend.delete_password.assert_called_with(...)orkeyring_mock.calls), replace the last two assertions intest_delete_http_password_with_dot_in_repo_name_keyring_enabledwith the appropriate expectation to verify that the keyring delete path was invoked.
- The tests above assume you already have fixtures named
-
If, in keyring-enabled mode,
PasswordManager.set_http_passworddoes not persist HTTP basic auth into config at all (only into keyring), update the "config entry should remain unchanged" assertion to instead verify that config has no stored credentials and rely solely on the keyring assertions.
Please align the fixtures (with_keyring, keyring_backend) and the exact assertions with how keyring is currently tested elsewhere in tests/utils/test_password_manager.py.
| def test_set_pypi_token_with_dot_in_repo_name( | ||
| config: Config, with_fail_keyring: None | ||
| ) -> None: | ||
| manager = PasswordManager(config) | ||
| manager.set_pypi_token("foo.bar", "baz") | ||
|
|
||
| assert config.get("pypi-token.foo\\.bar") == "baz" |
There was a problem hiding this comment.
suggestion (testing): Add complementary tests for get/delete pypi token and keyring mode with dotted repository names
To cover the new behavior more completely, please also add tests for:
get_pypi_token("foo.bar")when the token is stored in config (no keyring), asserting it reads from the escaped key.delete_pypi_token("foo.bar"), asserting it removes the escaped key.- A case where the keyring is available (without
with_fail_keyring), confirming dotted repo names still work and the escaping logic doesn’t affect the keyring path.
These will exercise all code paths for dotted repository names.
Suggested implementation:
def test_get_pypi_token_with_dot_in_repo_name_from_config(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
# Simulate an existing config entry using the escaped key
config["pypi-token.foo\\.bar"] = "baz"
token = manager.get_pypi_token("foo.bar")
assert token == "baz"
def test_delete_pypi_token_with_dot_in_repo_name_from_config(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
config["pypi-token.foo\\.bar"] = "baz"
manager.delete_pypi_token("foo.bar")
# Escaped key should be removed from config
assert "pypi-token.foo\\.bar" not in config
def test_pypi_token_with_dot_in_repo_name_with_keyring(
config: Config, monkeypatch: pytest.MonkeyPatch
) -> None:
manager = PasswordManager(config)
# Use a dummy in-memory keyring to observe key usage
stored: dict[tuple[str, str], str] = {}
class DummyKeyring:
def set_password(self, system: str, username: str, password: str) -> None:
stored[(system, username)] = password
def get_password(self, system: str, username: str) -> str | None:
return stored.get((system, username))
def delete_password(self, system: str, username: str) -> None:
stored.pop((system, username), None)
dummy = DummyKeyring()
# Patch the keyring used by PasswordManager so keyring is "available"
monkeypatch.setattr("poetry.utils.password_manager.keyring", dummy)
manager.set_pypi_token("foo.bar", "baz")
# Ensure the dotted repo name is used as-is in the keyring username
assert any(username == "foo.bar" for (_system, username) in stored.keys())
token = manager.get_pypi_token("foo.bar")
assert token == "baz"
manager.delete_pypi_token("foo.bar")
# After deletion, the key should be removed from the dummy keyring
assert not any(username == "foo.bar" for (_system, username) in stored.keys())
def test_set_http_password_with_dot_in_repo_name(
config: Config, with_fail_keyring: None
) -> None:
manager = PasswordManager(config)
manager.set_http_password("foo.bar", "baz", "qux")
auth = config.get("http-basic.foo\\.bar")
assert auth["username"] == "baz"
assert auth["password"] == "qux"- If
PasswordManagerimportskeyringfrom a different module path than"poetry.utils.password_manager", adjust themonkeypatch.setattrtarget accordingly so it patches the exact object used byPasswordManager. - If this test module does not yet import
pytest, addimport pytestat the top of the file (or extend an existing import) sopytest.MonkeyPatchand themonkeypatchfixture work. - If the project targets Python versions without support for
dict[tuple[str, str], str]type hints, change the annotation toDict[Tuple[str, str], str]and ensurefrom typing import Dict, Tupleis imported.
|
Thanks for the review details. I’ll add the missing dotted-repository get/delete coverage (including keyring paths where applicable) and push an update. |
|
Pushed follow-up tests for dotted repository names to cover read/delete paths end-to-end:
Validation:
|
9048b89 to
0efe6c1
Compare
|
Rebased this branch onto current I attempted local targeted verification, but this repo's test environment in my runner is missing several test dependencies ( Given the branch rebased cleanly with no conflict edits, CI should now provide the authoritative signal on this updated base. |
|
Follow-up pushed in Local check run on touched files:
|
Signed-off-by: Louis Shawn <louis.shawn@qq.com>
|
Pushed a follow-up fix for the remaining pytest failures in this branch:
Local verification:
|
Summary
foo\.barstays one segment)repositories.<name>.urlhttp-basic.<name>/pypi-token.<name>certificates.<name>.*poetry configFixes #1328.
Testing
pytest -q -o addopts='' tests/config/test_dict_config_source.py tests/config/test_file_config_source.py tests/utils/test_password_manager.pyNotes
tests/console/commands/test_config.pyincludes a new dotted-repository test, but this file could not be run locally in this environment becausedeepdiffis missing.Summary by Sourcery
Support configuration keys and repository names containing dots by introducing escaped-dot handling in config key parsing and applying it across repository, certificate, and credential configuration.
Bug Fixes:
Enhancements:
Tests: