Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds feature-gated validation metadata to credential fields, credential metadata entries, notification initialization parameters, and top-level serializer fields. Secret and non-string fields remain excluded, and shared notification parameters are copied before processing. ChangesValidation pattern metadata
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CredentialTypeSerializer
participant Metadata
participant validation_patterns
participant DAB
CredentialTypeSerializer->>validation_patterns: process credential fields and metadata
Metadata->>validation_patterns: process notification and top-level fields
validation_patterns->>DAB: build or inject validation patterns
DAB-->>validation_patterns: return validation metadata
validation_patterns-->>CredentialTypeSerializer: return updated credential schema
validation_patterns-->>Metadata: return updated API metadata
Merge Risk: ⚪ Minimal · up to This change adds feature-gated validation metadata to eligible API schema fields while excluding secret and non-string inputs. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Review: [AAP-87586] advertise validation patterns on JSON sub-key fieldsVerdict: READY_FOR_HUMAN_REVIEW No critical or major findings. This is a well-crafted, focused PR that adds metadata-only functionality with proper feature gating and comprehensive tests. Key Observations
Optional Polish (Nits — 0 points each)
Needs Human Judgment
Reviewed in API-only mode (no local checkout). Codebase-wide stale reference checks and verification commands were skipped. |
|
Overall looks good! Do we need to call inject_clean_text_patterns to get the validation passed into top-level fields here as well? |
I think this would probably be the right place to add that as well. Good call out |
Credential type inputs.fields/metadata and notification OPTIONS now include pattern and pattern_description for non-secret string sub-keys when ENHANCED_INPUT_VALIDATION_ENABLED is on.
Wires DAB's inject_clean_text_patterns into awx.api.metadata.Metadata so top-level CharField OPTIONS metadata gets pattern/patternDescription/flags, matching the JSON sub-key pattern exposure already in place. No-op until ENHANCED_INPUT_VALIDATION_ENABLED is on and a serializer mixes in CleanTextMixin. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com>
d67ddb6 to
2fdf290
Compare
vidyanambiar
left a comment
There was a problem hiding this comment.
Tested with aap-dev along with DAB PR 1119 and it works as expected. 👍
Import TIER2_PATTERN_DESCRIPTION from DAB instead of keeping a manually synced duplicate, and short-circuit inject_patterns_into_field_list before iterating fields when enhanced input validation is disabled. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com>
Shallow-copy each field before injecting pattern metadata so callers that pass module-level schema dicts (e.g. ManagedCredentialType.registry entries) don't get them mutated in place, matching the existing copy-before-mutate approach in inject_patterns_into_init_parameters. Signed-off-by: Pratyush Bhandari <prbhanda@redhat.com>
…s.py Align EDA's validation-pattern implementation with the AWX approach (ansible/awx#16638): - Create src/aap_eda/api/validation_patterns.py centralizing all pattern injection logic (inject_free_text_pattern, inject_patterns_into_field_list, inject_top_level_clean_text_patterns) - Use AWX-aligned DAB imports: build_tier2_frontend_pattern, inject_clean_text_patterns, TIER2_PATTERN_DESCRIPTION - Use copy.copy() before mutating shared field dicts instead of immutable list-comprehension approach - Add early-return gating (enhanced_input_validation_enabled) when validation is disabled - Simplify metadata.py and credential_type.py to delegate to the new validation_patterns module - Update unit and integration tests for the new module structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s.py Align EDA's validation-pattern implementation with the AWX approach (ansible/awx#16638): - Create src/aap_eda/api/validation_patterns.py centralizing all pattern injection logic (inject_free_text_pattern, inject_patterns_into_field_list, inject_top_level_clean_text_patterns) - Use AWX-aligned DAB imports: build_tier2_frontend_pattern, inject_clean_text_patterns, TIER2_PATTERN_DESCRIPTION - Use copy.copy() before mutating shared field dicts instead of immutable list-comprehension approach - Add early-return gating (enhanced_input_validation_enabled) when validation is disabled - Simplify metadata.py and credential_type.py to delegate to the new validation_patterns module - Update unit and integration tests for the new module structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f7d9ac6 to
d83bcfa
Compare
d83bcfa to
58ce353
Compare
|
❌ Test Results - FAILEDSummary
Pass Rate: 91.2% ❌ Failed Tests
|



SUMMARY
Advertise CleanTextMixin Tier 2 (
validate_free_text)pattern/pattern_descriptionon JSON sub-key schemas so API clients (AAP UI, AAP-87604) can show the same frontend hints used for top-level CharFields.This does not reject writes. Write-path sanitization is CleanTextMixin (AAP-78694). This change only adds metadata, gated by
ENHANCED_INPUT_VALIDATION_ENABLED.Injection points match how the UI already loads schemas:
GET /api/v2/credential_types/→inputs.fieldsGET /api/v2/credential_types/{id}/→inputs.metadataOPTIONS /api/v2/notification_templates/→actions.*.notification_configuration.{type}OPTIONS /credentials/and credential/CISinputsstay opaque JSON. Those keys depend oncredential_type, so the schema lives on the credential type, not the instance.Secret / password-typed / non-string sub-keys are skipped. Notification
init_parametersare deep-copied before injection so class-level backend dicts are never mutated.Depends on django-ansible-base#1119 (AAP-85987) for
build_tier2_frontend_pattern. Until that lands on DABdevel, injection is a no-op (ImportError) so Controller still imports and runs against current DAB.Related: AAP-87586
ISSUE TYPE
COMPONENT NAME
STEPS TO REPRODUCE AND EXTRA INFO
With
ENHANCED_INPUT_VALIDATION_ENABLED=trueand DAB PR 1119 available:GET /api/v2/credential_types/— non-secret stringinputs.fieldsincludepattern,pattern_description, andflags: i. Secret fields (e.g. password, ssh key, token) do not.GET /api/v2/credential_types/{id}/for an external type — same keys oninputs.metadatastring fields.OPTIONS /api/v2/notification_templates/— string keys undernotification_configuration.email(host, username, sender, …) include the pattern;password,port, and list fields do not.With the toggle off, none of those schemas include
pattern/pattern_description.Summary by CodeRabbit
ENHANCED_INPUT_VALIDATION_ENABLEDsetting.