[DRAFT] Add PostgreSQL utility functions for metrics-utility#16458
Open
cshiels-ie wants to merge 2 commits into
Open
[DRAFT] Add PostgreSQL utility functions for metrics-utility#16458cshiels-ie wants to merge 2 commits into
cshiels-ie wants to merge 2 commits into
Conversation
Creates two plpgsql functions used by metrics-utility when querying the AWX controller database via a read-only connection: - metrics_utility_parse_yaml_field: extracts a named field from YAML-encoded text - metrics_utility_is_valid_json: validates whether a text value is valid JSON GRANT EXECUTE TO PUBLIC so any database user (including ms_awx_readonly) can call the functions without requiring an explicit per-user grant. The migration is a no-op on SQLite so unit and functional tests are unaffected. Assisted-by: Claude Code (claude-sonnet-4-6) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ciaran Shiels <cas.ireland@Gmail.com>
📝 WalkthroughWalkthroughThis PR introduces a single Django database migration that creates two PostgreSQL PL/pgSQL utility functions for metrics processing. The functions extract YAML field values and validate JSON input, with PUBLIC execute grants for accessibility. The migration includes rollback SQL to drop both functions if reversed and treats SQLite as a no-op. ChangesMetrics Utility Database Functions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
awx/main/migrations/0206_create_metrics_utility_functions.py (1)
29-31: ⚡ Quick winNarrow exception handling in JSON validation.
WHEN others THEN RETURN falsehides every runtime error, not just invalid JSON input, making operational debugging harder and masking unexpected failures. Catch only the JSON-cast exception(s) expected for invalid input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@awx/main/migrations/0206_create_metrics_utility_functions.py` around lines 29 - 31, Replace the broad "EXCEPTION WHEN others THEN RETURN false;" with a narrow handler that only catches the JSON parse error (SQLSTATE 22P02) so other runtime errors still surface; i.e., change the EXCEPTION block to "WHEN SQLSTATE '22P02' THEN RETURN false;" (or include any additional specific JSON-related SQLSTATE codes you expect) so only invalid JSON returns false and all other exceptions propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@awx/main/migrations/0206_create_metrics_utility_functions.py`:
- Around line 16-18: The regex injection risk is in the construction of line_re
using the unescaped variable field (variables field_re and line_re and the
substring(str from line_re) call) — escape or regexp-quote field before
concatenating so meta-characters in field cannot alter the regex; replace the
broad EXCEPTION WHEN others THEN RETURN false in the JSON-cast helper with a
targeted exception handler that catches the specific
JSON/invalid_text_representation error only; and remove the blanket GRANT
EXECUTE TO PUBLIC, granting the helper functions only to the specific role(s)
that actually need them (update the GRANT statements accordingly).
- Around line 36-37: Replace the broad "GRANT EXECUTE ... TO PUBLIC" for the
functions metrics_utility_parse_yaml_field(text, text) and
metrics_utility_is_valid_json(text) with grants targeted to the AWX metrics
role(s) that actually run metrics queries (e.g., the read-only metrics role);
update the two GRANT lines so they grant EXECUTE only to that role (and remove
or revert the TO PUBLIC grants), ensuring the migrations call GRANT EXECUTE ON
FUNCTION metrics_utility_parse_yaml_field(text, text) TO <metrics_readonly_role>
and GRANT EXECUTE ON FUNCTION metrics_utility_is_valid_json(text) TO
<metrics_readonly_role> (use the project’s canonical metrics role name).
---
Nitpick comments:
In `@awx/main/migrations/0206_create_metrics_utility_functions.py`:
- Around line 29-31: Replace the broad "EXCEPTION WHEN others THEN RETURN
false;" with a narrow handler that only catches the JSON parse error (SQLSTATE
22P02) so other runtime errors still surface; i.e., change the EXCEPTION block
to "WHEN SQLSTATE '22P02' THEN RETURN false;" (or include any additional
specific JSON-related SQLSTATE codes you expect) so only invalid JSON returns
false and all other exceptions propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: acf97d71-be94-4d74-be58-ca0fa4d69793
📒 Files selected for processing (1)
awx/main/migrations/0206_create_metrics_utility_functions.py
- Escape `field` parameter before interpolating into regex pattern using regexp_replace with the whole-match `&` reference rather than bare concatenation; prevents regex metacharacters in the field name from altering match semantics - Narrow `EXCEPTION WHEN others` to `WHEN invalid_text_representation` (SQLSTATE 22P02) in metrics_utility_is_valid_json so only the expected JSON-cast failure is swallowed and unexpected runtime errors propagate Assisted-by: Claude Code (claude-sonnet-4-6) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ciaran Shiels <cshiels@redhat.com>
|
The That could be a historical artifact now .. or not :). We'll need to test what happens when the saved json actually uses the Anyway it might be something to incorporate into these functions directly, if it's still a problem. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
SUMMARY
Adds a new migration (
0206) that creates two plpgsql helper functions in the AWX controller database used bymetrics-utilitywhen querying via a read-only connection:metrics_utility_parse_yaml_field(str text, field text)— extracts a named field from YAML-encoded text using a regex match; the field name is escaped before interpolation to prevent regex metacharacter injectionmetrics_utility_is_valid_json(p_json text)— validates whether a text value is parseable as JSON, catching onlyinvalid_text_representation(SQLSTATE 22P02)Grants
EXECUTE TO PUBLICso any database user (including thems_awx_readonlymetrics read-only user) can call the functions without an explicit per-user grant. The metrics read user is customer-configurable, so a specific role name cannot be hardcoded.Uses
dbawaremigrations.RunSQLwithsqlite_sql=noopso unit and functional tests (which use SQLite) are unaffected.ISSUE TYPE
COMPONENT NAME
STEPS TO REPRODUCE AND EXTRA INFO
Test plan:
make testpasses (SQLite path, noop)make test_migrationspassesawx-manage migratecreates both functions (\df metrics_utility*)SELECT metrics_utility_is_valid_json('{"a":1}');returnstrueSELECT metrics_utility_parse_yaml_field('name: tower', 'name');returnstowerms_awx_readonlyuser can execute both functionsAI Assistance
This change was developed with assistance from Claude Code (Claude Sonnet 4.6).
All generated code was reviewed, tested, and validated before merging.