-
Notifications
You must be signed in to change notification settings - Fork 10
Open
Description
There is currently a potential bug risk in the handling of the settings configuration parameter. The code assumes that settings is always a dictionary and passes it directly to update(). If a user provides a non-dict value (e.g., a list or string) in their config file, this will result in a runtime error, but the resulting stack trace may be deep within the codebase and not clearly indicate that the root cause is an invalid type for settings.
Proposed Solution:
- Add an explicit type check for
settingswhen loading the configuration. Ifsettingsis not a dictionary, raise a clear and descriptive error (e.g.,ConfigurationError) that tells the user exactly what went wrong and how to fix it. - Alternatively, consider defaulting to an empty dict if the type is incorrect, but raising an error is preferable for clarity and to avoid silently swallowing misconfigurations.
Example Implementation:
settings = self._ch_ctl_config.get("settings", {})
if not isinstance(settings, dict):
raise ConfigurationError(
f"Invalid type for `clickhouse.settings`: expected dict, got {type(settings).__name__}"
)Long-term Suggestion:
- Consider using a strongly-typed configuration library such as Pydantic to enforce config types at load time. This would provide even more robust validation, but is likely out of scope for the current PR.
Action Items:
- Add an explicit type check for
settingsin the configuration loading logic. - Raise a clear error if the type is incorrect.
- Optionally, create a follow-up issue to investigate using Pydantic or similar for full config validation.
I created this issue for @Alex-Burmak from #248 (comment).
Tips and commands
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels