-
Notifications
You must be signed in to change notification settings - Fork 89
[ENG-2185] #7133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
[ENG-2185] #7133
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
45ad401
initial
45396d4
added jsonb col and updated tests
c83780f
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
JadeCara a89bbce
update db yml
f5cf36a
test updates
fdef91d
clean up alembic file
2c3bc15
remove extra definition
9a788df
remove extra definition
fccc79b
fix return
ae56487
some clean ups
57599c9
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
JadeCara 10c24be
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
744aa5e
missed one
0400766
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
JadeCara e025468
fix tests
b9f38c5
Merge branch 'ENG-2185-add-new-jsob-tree-col' of github.com:ethyca/fi…
3891837
clean up
f889e04
clean up
bf574bd
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
JadeCara 68d1c4e
fix
a774f76
.
eb5d0aa
Apply suggestion from @JadeCara
JadeCara 43a06fc
small udpates
39037fe
small docstring clean ups
3e07cce
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
JadeCara ee09dbd
changed get_root_condition to get_condition_tree
e952c1b
move from raw string to constant
ba6556f
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
JadeCara d49d3da
Merge branch 'main' into ENG-2185-add-new-jsob-tree-col
JadeCara b6cf8a8
changelog
8d17149
Apply suggestion from @JadeCara
JadeCara 40265f0
Apply suggestion from @JadeCara
JadeCara b2273a4
remove redundant test
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # Copy this file and rename it (e.g., pr-number.yaml or feature-name.yaml) | ||
| # Fill in the required fields and delete this comment block | ||
|
|
||
| type: Added # One of: Added, Changed, Developer Experience, Deprecated, Docs, Fixed, Removed, Security | ||
| description: Added a new jsonb column to the conditional dependency models. | ||
| pr: 7133 # PR number | ||
| labels: ["db-migration"] # Optional: ["high-risk", "db-migration"] |
182 changes: 182 additions & 0 deletions
182
.../api/alembic/migrations/versions/xx_2025_12_16_1630_85ce2c1c9579_add_jsonb_tree_column.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| """add jsonb tree column | ||
|
|
||
| Revision ID: 85ce2c1c9579 | ||
| Revises: b9c8e7f6d5a4 | ||
| Create Date: 2025-12-16 16:30:52.073758 | ||
|
|
||
| """ | ||
|
|
||
| import json | ||
| from typing import Optional | ||
|
|
||
| import sqlalchemy as sa | ||
| from alembic import op | ||
| from sqlalchemy.dialects import postgresql | ||
| from sqlalchemy.orm import Session | ||
| from sqlalchemy.sql import quoted_name | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = "85ce2c1c9579" | ||
| down_revision = "b9c8e7f6d5a4" | ||
| branch_labels = None | ||
| depends_on = None | ||
|
|
||
| # Whitelist of allowed table names for this migration | ||
| ALLOWED_TABLES = frozenset({"manual_task_conditional_dependency", "digest_condition"}) | ||
| ALLOWED_ID_COLUMNS = frozenset({"id"}) | ||
|
|
||
|
|
||
| def _validate_identifier(identifier: str, allowed: frozenset, identifier_type: str) -> str: | ||
| """Validate that an identifier is in the allowed whitelist. | ||
|
|
||
| Args: | ||
| identifier: The table or column name to validate | ||
| allowed: Set of allowed identifier values | ||
| identifier_type: Description of the identifier type for error messages | ||
|
|
||
| Returns: | ||
| The validated identifier | ||
|
|
||
| Raises: | ||
| ValueError: If the identifier is not in the allowed set | ||
| """ | ||
| if identifier not in allowed: | ||
| raise ValueError( | ||
| f"Invalid {identifier_type}: {identifier!r}. " | ||
| f"Allowed values: {sorted(allowed)}" | ||
| ) | ||
| return identifier | ||
|
|
||
|
|
||
| def _safe_identifier(name: str) -> quoted_name: | ||
| """Return a properly quoted SQL identifier.""" | ||
| return quoted_name(name, quote=True) | ||
|
|
||
|
|
||
| def build_condition_tree( | ||
| db: Session, table_name: str, row_id: str, id_column: str = "id" | ||
| ) -> Optional[dict]: | ||
| """Recursively build a condition tree from row-based storage. | ||
|
|
||
| Args: | ||
| db: Database session | ||
| table_name: Name of the table (must be in ALLOWED_TABLES whitelist) | ||
| row_id: ID of the row to build tree from | ||
| id_column: Name of the ID column (must be in ALLOWED_ID_COLUMNS whitelist) | ||
|
|
||
| Returns: | ||
| dict: Condition tree as a dictionary (ConditionLeaf or ConditionGroup format) | ||
|
|
||
| Raises: | ||
| ValueError: If table_name or id_column is not in the allowed whitelist | ||
| """ | ||
| # Validate identifiers against whitelist to prevent SQL injection | ||
| _validate_identifier(table_name, ALLOWED_TABLES, "table name") | ||
| _validate_identifier(id_column, ALLOWED_ID_COLUMNS, "id column") | ||
|
|
||
| # Use quoted identifiers for defense in depth | ||
| safe_table = _safe_identifier(table_name) | ||
| safe_id_col = _safe_identifier(id_column) | ||
|
|
||
| result = db.execute( | ||
| sa.text( | ||
| f"SELECT condition_type, field_address, operator, value, logical_operator " | ||
| f"FROM {safe_table} WHERE {safe_id_col} = :row_id" | ||
| ), | ||
| {"row_id": row_id}, | ||
| ).fetchone() | ||
|
|
||
| if not result: | ||
| return None | ||
|
|
||
| condition_type, field_address, operator, value, logical_operator = result | ||
|
|
||
| if condition_type == "leaf": | ||
| parsed_value = value | ||
|
|
||
| return { | ||
| "field_address": field_address, | ||
| "operator": operator, | ||
| "value": parsed_value, | ||
| } | ||
|
JadeCara marked this conversation as resolved.
|
||
|
|
||
| # It's a group - get children ordered by sort_order | ||
| children_rows = db.execute( | ||
| sa.text( | ||
| f"SELECT {safe_id_col} FROM {safe_table} " | ||
| f"WHERE parent_id = :parent_id ORDER BY sort_order" | ||
| ), | ||
| {"parent_id": row_id}, | ||
| ).fetchall() | ||
|
|
||
| child_conditions = [] | ||
| for (child_id,) in children_rows: | ||
| child_tree = build_condition_tree(db, table_name, child_id, id_column) | ||
| if child_tree: | ||
| child_conditions.append(child_tree) | ||
|
|
||
| if not child_conditions: | ||
| return None | ||
|
|
||
| return { | ||
| "logical_operator": logical_operator, | ||
| "conditions": child_conditions, | ||
| } | ||
|
|
||
|
|
||
| def migrate_conditions(db: Session, table_name: str) -> None: | ||
| """Migrate existing row-based condition trees to JSONB format for the given table. | ||
|
|
||
| Args: | ||
| db: Database session | ||
| table_name: Name of the table (must be in ALLOWED_TABLES whitelist) | ||
|
|
||
| Raises: | ||
| ValueError: If table_name is not in the allowed whitelist | ||
| """ | ||
| # Validate table name against whitelist | ||
| _validate_identifier(table_name, ALLOWED_TABLES, "table name") | ||
| safe_table = _safe_identifier(table_name) | ||
|
|
||
| root_rows = db.execute( | ||
| sa.text(f"SELECT id FROM {safe_table} WHERE parent_id IS NULL") | ||
| ).fetchall() | ||
|
|
||
| for (root_id,) in root_rows: | ||
| tree = build_condition_tree(db, table_name, root_id) | ||
|
|
||
| if tree: | ||
| db.execute( | ||
| sa.text( | ||
| f"UPDATE {safe_table} " | ||
| "SET condition_tree = :tree WHERE id = :root_id" | ||
| ), | ||
| {"tree": json.dumps(tree), "root_id": root_id}, | ||
| ) | ||
|
|
||
|
|
||
| def upgrade(): | ||
| # Step 1: Add condition_tree column to both tables | ||
| op.add_column( | ||
| "digest_condition", | ||
| sa.Column( | ||
| "condition_tree", postgresql.JSONB(astext_type=sa.Text()), nullable=True | ||
| ), | ||
| ) | ||
| op.add_column( | ||
| "manual_task_conditional_dependency", | ||
| sa.Column( | ||
| "condition_tree", postgresql.JSONB(astext_type=sa.Text()), nullable=True | ||
| ), | ||
| ) | ||
|
|
||
| # Step 2: Migrate existing row-based trees to JSONB | ||
| db = Session(op.get_bind()) | ||
| migrate_conditions(db, "manual_task_conditional_dependency") | ||
| migrate_conditions(db, "digest_condition") | ||
|
JadeCara marked this conversation as resolved.
JadeCara marked this conversation as resolved.
|
||
| db.commit() | ||
|
|
||
|
|
||
| def downgrade(): | ||
| op.drop_column("manual_task_conditional_dependency", "condition_tree") | ||
| op.drop_column("digest_condition", "condition_tree") | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.