feat: [SNOW-2457402] Unify encoding support in SnowCLI#2792
feat: [SNOW-2457402] Unify encoding support in SnowCLI#2792sfc-gh-kolszewski wants to merge 17 commits intomainfrom
Conversation
sfc-gh-olorek
left a comment
sfc-gh-olorek
left a comment
There was a problem hiding this comment.
Three issues found during review — fixes committed on the branch.
|
|
||
| DEFAULT_TIMEOUT = 30 | ||
| DRIVER_PATH = Path(__file__).parent / "setup_driver.py.source" | ||
| DRIVER_PATH = SecurePath(__file__).parent / "setup_driver.py.source" |
There was a problem hiding this comment.
Bug: DRIVER_PATH changed from Path to SecurePath, but DRIVER_PATH.read_text() at line 169 is called without the required file_size_limit_mb argument — this is a TypeError at runtime.
Since this is an internal bundled script with no encoding concern, reverting back to Path.
| feature_flag, | ||
| ): | ||
| mock_get_config_value.return_value = feature_flag | ||
| mock_get_config_value.side_effect = [None, feature_flag] |
There was a problem hiding this comment.
Fragile: side_effect = [None, feature_flag] is order-dependent. If any additional get_config_value call is added to the production code path (e.g. another encoding config read), the list shifts and the test silently returns wrong values.
Replacing with a function-based side_effect that matches on key name, so it's resilient to changes in the number of config reads.
| def _read_config_file_toml() -> dict: | ||
| return tomlkit.loads(get_config_manager().file_path.read_text()).unwrap() | ||
| return tomlkit.loads( | ||
| SecurePath(get_config_manager().file_path).read_text( |
There was a problem hiding this comment.
TOML files are always UTF-8 by spec. Since SecurePath.read_text() now applies the user's file_io encoding setting, a user who configures file_io = "cp1252" would corrupt the CLI's own config file on re-read.
Same issue applies to _read_connections_toml and _update_connections_toml below.
Adding explicit encoding="utf-8" to all three functions.
… encoding - Revert DRIVER_PATH from SecurePath back to Path: SecurePath.read_text() requires file_size_limit_mb which was not passed, causing a runtime crash. The bundled driver script has no encoding concern. - Use function-based side_effect in test_manager.py instead of fragile ordered list [None, feature_flag] that breaks when config reads change. - Always read/write TOML config files with encoding="utf-8" since TOML spec requires UTF-8. Prevents user's file_io encoding config from corrupting the CLI's own config and connections files.
sfc-gh-olorek
force-pushed
the
ko/SNOW-2457402-unify-encoding-support
branch
from
3a9a274 to
4ee3e72
Compare
2 participants
Pre-review checklist
Changes description
See https://snowflakecomputing.atlassian.net/browse/SNOW-2683933
This change adds new encoding configuration section with following keys:
Encoding settings are read from the configuration by SecurePath's read_text, write_text and open methods. As such, most places in the code where SecurePath was not used for these purposes were refactored to use SecurePath. I did not apply this change where encoding was already explicitly set as 'utf-8'
The change should be transparent to Unix users. Windows users can expect the warning to be displayed.