NAS-135965 / 25.10 / Move apps config schema handling from middleware classes to pydantic #16555
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bugclerk
changed the title
creatorcary
reviewed
May 27, 2025
sonicaj
force-pushed
the
NAS-135965
branch
2 times, most recently
from
c13b987 to
d4f2332
Compare
sonicaj
force-pushed
the
NAS-135965
branch
4 times, most recently
from
9e2055d to
0d94ea8
Compare
sonicaj
force-pushed
the
NAS-135965
branch
5 times, most recently
from
13d2f6d to
6e667d9
Compare
- Add create_length_validated_hostpath function to apply min/max length validation before path conversion - Update schema construction to use length-validated hostpath when constraints are specified - Add comprehensive tests for path and hostpath types with length constraints - Fix test expectations to properly validate empty strings against length constraints - Update test assertions for path type to expect 'items' in error messages - Fix code quality issues (trailing whitespace, blank lines) This ensures hostpath fields properly validate string length before converting to Path objects, preventing TypeError when length constraints are applied.
This reverts commit 2217062. The original behavior where non-required lists and dicts automatically get empty defaults ([] and {}) was the intended behavior for backward compatibility with existing app configurations. This commit restores: - default_factory assignment for nested dicts in process_schema_field - default_factory set to list/dict for non-required lists/dicts in create_field_info_from_schema - Test expectations that non-required lists/dicts get empty defaults The behavior ensures that: - Non-required lists without explicit defaults get [] - Non-required dicts without explicit defaults get {} - Nested dicts with default values on all subfields get instantiated with those defaults
Previously, validation errors for nested fields were prefixed with 'values.'
(e.g., 'values.database.host' instead of just 'database.host'). This made
error messages less clear and inconsistent with field naming.
Changes:
- Modified construct_schema() to use verrors.extend() instead of
verrors.add_child('values', e) to avoid adding the prefix
- Added comprehensive tests for nested validation error paths
- Updated existing tests to expect clean paths without prefix
This improves error reporting clarity for deeply nested app configurations.
When a parent dict field is hidden by a show_if condition, its required nested fields should not trigger validation errors. This fix propagates the visibility state from parent dict fields to their children during Pydantic model generation. The implementation: - Adds parent_hidden parameter to generate_pydantic_model to track visibility - Evaluates show_if conditions for dict fields and propagates hidden state - Marks all fields as NotRequired when their parent dict is hidden - Preserves existing show_if behavior for non-nested cases This resolves the issue where required fields inside hidden dict structures (e.g., storage.data.host_path_config.path) would still cause validation errors even when their parent was hidden by show_if.
- Fixed test_normalize_and_validate by properly mocking validate_values to return a mutable dictionary instead of a Dict schema object - Removed unused pytest import from test_nested_show_if.py - Fixed whitespace issues (W293, W291, W292) in both modified files - All tests now pass successfully
When evaluating show_if conditions, the code now properly builds an evaluation context that includes default values from sibling fields. This fixes the issue where fields with show_if conditions inside hidden parent dicts would still cause validation errors. The fix addresses the actual-budget app installation issue where the path field was required even though its parent host_path_config was hidden due to type='ix_volume' (default). Changes: - Build eval_context with both provided values and defaults - Use eval_context for show_if evaluation instead of raw new_values - Added test case for nested show_if with field-level conditions - Fixed flake8 whitespace issues
When creating apps with empty values {}, the schema validation was using
exclude_unset=True which prevented defaults from being populated. This
caused template rendering to fail when accessing fields like run_as.user.
Changed construct_schema to use exclude_unset=False so defaults are properly
populated for fields not provided by the user.
- Updated validate_model call to exclude_unset=False in construct_schema
- Added test to verify defaults are populated with empty input
- Updated test_normalize_and_validate to use actual validation logic
- Fix remove_not_required() to handle string representations of NotRequired objects that appear when using dynamic Pydantic models - Add comprehensive test coverage for default values and NotRequired handling - Ensure empty dicts/lists are populated for non-required fields without defaults - Allow explicitly provided values to override show_if conditions - Update test expectations to match correct behavior where: - Non-required dict fields get empty dict defaults - Non-required list fields get empty list defaults - Explicitly provided values are included even when show_if=false - Fields with defaults are populated when not provided by user This ensures consistent behavior for app schema validation and proper handling of optional fields across all schema types.
creatorcary
reviewed
Jul 2, 2025
src/middlewared/middlewared/plugins/apps/schema_construction_utils.py
Outdated
Show resolved
Hide resolved
src/middlewared/middlewared/plugins/apps/schema_construction_utils.py
Outdated
Show resolved
Hide resolved
src/middlewared/middlewared/plugins/apps/schema_construction_utils.py
Outdated
Show resolved
Hide resolved
src/middlewared/middlewared/plugins/apps/schema_construction_utils.py
Outdated
Show resolved
Hide resolved
src/middlewared/middlewared/plugins/apps/schema_construction_utils.py
Outdated
Show resolved
Hide resolved
| 'string', 'int', 'boolean', 'path' | ||
| ) and old_values is not NOT_PROVIDED: | ||
| # If we have a value for this field in old_values, we should not allow it to be changed | ||
| field_type = Literal[old_values] |
There was a problem hiding this comment.
This path should be tested. Passing a dictionary to Literal might not behave as expected.
There was a problem hiding this comment.
It won't be a dictionary and yes you are right, it won't behave as expected with a dict. It will be a simple types based off the and condition we have
yocalebo
approved these changes
Jul 2, 2025
creatorcary
approved these changes
Jul 3, 2025
|
Thanks! |
themylogin
approved these changes
Jul 4, 2025
|
This PR has been merged and conversations have been locked. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.