update ConfigModel to prevent api key output in a debug log#29
Conversation
…nd user interface factories
… for configuration
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors configuration handling to use strongly-typed Pydantic model instances instead of generic dictionaries throughout the application. The changes improve type safety by requiring factory functions to accept specific configuration model types rather than untyped dictionaries.
Key changes:
- Updated
ConfigModelto use Pydantic model instances for nested configuration fields instead of dictionaries - Modified factory functions to accept typed configuration models and use
.model_dump()for serialization - Updated all test cases to pass model instances instead of dictionaries to factory functions
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
simple_typing_application/models/config_models/general_config_model.py |
Changed nested config fields from dict types to union types of specific Pydantic models |
simple_typing_application/key_monitor/factory.py |
Updated create_key_monitor to accept BaseKeyMonitorConfigModel instead of dict |
simple_typing_application/sentence_generator/factory.py |
Updated create_sentence_generator to accept BaseSentenceGeneratorConfigModel instead of dict |
simple_typing_application/ui/factory.py |
Updated create_user_interface to accept BaseUserInterfaceConfigModel instead of dict |
tests/key_monitor/test_factory.py |
Updated test parameterization to use model instances; removed unused mocker parameter |
tests/sentence_generator/test_factory.py |
Updated test parameterization to use model instances |
tests/ui/test_factory.py |
Updated test parameterization to use model instances; removed unused mocker parameter |
tests/test_config.py |
Added API key handling logic to exclude sensitive fields from test assertions |
Comments suppressed due to low confidence (6)
tests/ui/test_factory.py:90
- The factory function signature was changed to accept a
BaseUserInterfaceConfigModelinstance instead of a dictionary. This test will fail because{}is not a validBaseUserInterfaceConfigModelinstance. Replace{}withBaseUserInterfaceConfigModel().
{},
tests/ui/test_factory.py:105
- The factory function signature was changed to accept a
BaseUserInterfaceConfigModelinstance instead of a dictionary. This test will fail because{}is not a validBaseUserInterfaceConfigModelinstance. Replace{}withBaseUserInterfaceConfigModel().
{},
tests/sentence_generator/test_factory.py:153
- The factory function signature was changed to accept a
BaseSentenceGeneratorConfigModelinstance instead of a dictionary. This test will fail because{}is not a validBaseSentenceGeneratorConfigModelinstance. Replace{}withBaseSentenceGeneratorConfigModel().
{},
tests/sentence_generator/test_factory.py:168
- The factory function signature was changed to accept a
BaseSentenceGeneratorConfigModelinstance instead of a dictionary. This test will fail because{}is not a validBaseSentenceGeneratorConfigModelinstance. Replace{}withBaseSentenceGeneratorConfigModel().
{},
tests/key_monitor/test_factory.py:91
- The factory function signature was changed to accept a
BaseKeyMonitorConfigModelinstance instead of a dictionary. This test will fail because{}is not a validBaseKeyMonitorConfigModelinstance. Replace{}withBaseKeyMonitorConfigModel().
{},
tests/key_monitor/test_factory.py:106
- The factory function signature was changed to accept a
BaseKeyMonitorConfigModelinstance instead of a dictionary. This test will fail because{}is not a validBaseKeyMonitorConfigModelinstance. Replace{}withBaseKeyMonitorConfigModel().
{},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ix-the-issue-of-log-content
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key_monitor_type: EKeyMonitorType = EKeyMonitorType.PYNPUT | ||
| key_monitor_config: dict[str, str | float | int | None | dict | list] = BaseKeyMonitorConfigModel().model_dump() # noqa | ||
| key_monitor_config: ( | ||
| BaseKeyMonitorConfigModel | PynputBasedKeyMonitorConfigModel | SSHKeyboardBasedKeyMonitorConfigModel |
There was a problem hiding this comment.
[nitpick] Inconsistent formatting: The union types for key_monitor_config are all on one line (line 40), while sentence_generator_config splits each union type onto separate lines (lines 27-30). For consistency and readability, consider formatting key_monitor_config the same way:
key_monitor_config: (
BaseKeyMonitorConfigModel
| PynputBasedKeyMonitorConfigModel
| SSHKeyboardBasedKeyMonitorConfigModel
) = BaseKeyMonitorConfigModel()| BaseKeyMonitorConfigModel | PynputBasedKeyMonitorConfigModel | SSHKeyboardBasedKeyMonitorConfigModel | |
| BaseKeyMonitorConfigModel | |
| | PynputBasedKeyMonitorConfigModel | |
| | SSHKeyboardBasedKeyMonitorConfigModel |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I found that debug logs might contains the OPENAI API KEY like
(b82784b). This is a security concern because sensitive configuration values such as
openai_api_keyare being logged in plain text via the debug logger.This pull request refactors the configuration handling throughout the application to use strongly-typed Pydantic model instances instead of generic dictionaries. This change improves type safety, code clarity, and maintainability across the core factory functions and their corresponding tests. The update also modifies how configuration models are passed and processed, requiring updates to parameter types and test setups.
Refactoring configuration handling
Updated the
ConfigModelinsimple_typing_application/models/config_models/general_config_model.pyto use specific Pydantic model instances forsentence_generator_config,user_interface_config, andkey_monitor_configfields, replacing the previous use of generic dictionaries.Refactored factory functions (
create_key_monitor,create_sentence_generator, andcreate_user_interface) to accept configuration model instances instead of dictionaries, and adjusted their internal logic to use.model_dump()for serialization. Changes applied insimple_typing_application/key_monitor/factory.py,simple_typing_application/sentence_generator/factory.py, andsimple_typing_application/ui/factory.py. [1] [2] [3] [4] [5] [6]Test updates
Updated all relevant test parameterizations and function signatures to pass configuration model instances instead of dictionaries, ensuring tests match the new function interfaces. Changes made in
tests/key_monitor/test_factory.py,tests/sentence_generator/test_factory.py, andtests/ui/test_factory.py. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Modified the configuration comparison logic in
tests/test_config.pyto exclude sensitive fields (likeopenai_api_key) from equality checks, reflecting the new model-based configuration structure.Check
uv run python -m simple_typing_applicationoutputs a debug log: