feat(clp-package): Let clp-config objects check against unexpected fields. #676
Conversation
WalkthroughThe changes involve introducing a new Changes
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/clp-py-utils/clp_py_utils/clp_config.py (3)
Line range hint
318-334: Consider enhancing error messages for credential validation.While the validation is robust, the error messages could be more helpful when dealing with credential configurations. For example, when a user mistypes "access_secret_key" instead of "secret_access_key", they should receive a clear message suggesting the correct field name.
class S3Credentials(BaseModelForbidExtra): access_key_id: str secret_access_key: str @validator("access_key_id") def validate_access_key_id(cls, field): if field == "": - raise ValueError("access_key_id cannot be empty") + raise ValueError("S3 access_key_id cannot be empty") return field @validator("secret_access_key") def validate_secret_access_key(cls, field): if field == "": - raise ValueError("secret_access_key cannot be empty") + raise ValueError("S3 secret_access_key cannot be empty. Note: If you specified 'access_secret_key', this is a typo - the correct field name is 'secret_access_key'") return fieldAlso applies to: 335-367
Line range hint
546-685: Consider consolidating validation methods in CLPConfig.The class has multiple separate validation methods that are called independently. Consider creating a single
validate_all()method that ensures all validations are performed in the correct order.class CLPConfig(BaseModelForbidExtra): # ... existing code ... + + def validate_all(self, clp_home: pathlib.Path): + """ + Performs all validations in the correct order. + Raises ValueError if any validation fails. + """ + self.make_config_paths_absolute(clp_home) + self.validate_input_logs_dir() + self.validate_archive_output_config() + self.validate_stream_output_dir() + self.validate_data_dir() + self.validate_logs_dir()
Line range hint
1-700: Consider reorganizing the configuration classes for better maintainability.The file has grown quite large with many configuration classes. Consider:
- Moving shared validation logic (e.g.,
_validate_logging_level,_validate_host) to a separatevalidators.pymodule- Grouping related configurations (e.g., storage-related, worker-related) into separate modules
This would improve maintainability and make the codebase more modular.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/clp_config.py(20 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
60-63: Well-designed base class implementation!The
BaseModelForbidExtraclass effectively implements the validation of unexpected fields, which will help catch configuration errors early and prevent silent failures.
|
Actually it might be an overkill to let all config object to inherit this new class BaseClassForbitExtra. |
Description
Currently clp-config doesn't enforce what fields are allowed in the config object.
This could be error prone when user specifies a value with None as default.
For example, if user forgets to indent the "credentials" config, clp_config will sliently ignore it and assign a default=None to the credentials under the s3_config. As a result, the package returns an no credentials error at later stage of compression/search.
Similarly, user could make a typo in the field name and as a result the field will be silently ignored.
This PR introduces a new base class with config that forbids extra field, so that all other config objects can simply inherit from this bass class and automatically check against unexpected field.
Known issue:
The error message can gets confusing if user makes a mistake under
storage config.For example, given the following config with a typo in "access_secret_key" field:
User will see error message:
While I don't entirely understand the behavior, here is my guess:
The latter 2 messages are printed when pydantic maps the storage config into S3Storage config, and it found that
secret_access_keyis missing, andaccess_key_sectis an unexpected fieldThen, pydantic tries to map the storage config into FsStorage, where it prints the first 3 messages, because:
Validation performed
Manually validated.
Summary by CodeRabbit