Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions api/survey_engine/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,117 @@ def _validate_visible_if(definition: dict[str, Any], question_names: set[str]) -
)


def _validate_construct_string(label: str, key: str, value: Any) -> None:
"""A construct_block / construct_item value, if set, is a non-empty string."""
if value is None:
return
if not isinstance(value, str) or not value.strip():
raise InvalidDefinition(
f"question {label!r}: {key} must be a non-empty string when set, got {value!r}"
)


def _validate_construct_tags(definition: dict[str, Any]) -> None:
"""Construct-membership tags. Two optional custom attributes:

construct_block: identifier of a reusable scale (e.g. 'phq9', 'gad7').
construct_item: identifier of an item within that block ('phq9_q3').

Where they may appear:
- Top-level plain question: both, no inheritance.
- matrix / paneldynamic container: block only (inherited by leaves).
- Matrix row, matrixdropdown row,
paneldynamic template element: both; block falls back to the
container's value if unset.

Authoring rules enforced here so a clear 422 reaches the editor (FR-2),
rather than the value rotting silently into the warehouse:
- Each tag is a non-empty string if set.
- A container element (matrix / paneldynamic) does not carry
construct_item: items belong to leaf questions (rows / cells),
not to the container that groups them.
- A leaf carrying construct_item has a construct_block in scope —
either its own or inherited from the container.

Note: these tags are *provenance*, not analytical-pooling instructions.
Cross-version / cross-survey pooling stays the explicit parent_question_id
opt-in (invariant 5). Two questions sharing a construct_block does not
license `GROUP BY construct_block` for pooled analysis — that is still a
deliberate researcher judgment expressed via parent_question_id.
"""
for element in _iter_elements(definition):
name = element.get("name")
if not isinstance(name, str) or not name:
# Display-only elements have no question identity; skip.
continue
_validate_construct_string(name, "construct_block", element.get("construct_block"))
_validate_construct_string(name, "construct_item", element.get("construct_item"))
owner_block = element.get("construct_block")
owner_item = element.get("construct_item")
qtype = element.get("type")

if qtype in MATRIX_TYPES:
if isinstance(owner_item, str):
raise InvalidDefinition(
f"question {name!r}: construct_item belongs to a matrix row, "
"not the matrix itself — move it onto the row"
)
for row in element.get("rows", []) or []:
Comment on lines +609 to +615
if not isinstance(row, dict):
# A scalar row (string id) cannot carry construct attrs; skip.
continue
row_id = _choice_value(row) or "<row>"
row_label = f"{name}.{row_id}"
_validate_construct_string(row_label, "construct_block", row.get("construct_block"))
_validate_construct_string(row_label, "construct_item", row.get("construct_item"))
row_block = row.get("construct_block") or owner_block
if isinstance(row.get("construct_item"), str) and not isinstance(row_block, str):
raise InvalidDefinition(
f"question {row_label!r}: construct_item set but no construct_block "
"in scope (set it on the row or the matrix)"
)
# matrixdropdown columns: a (row, col) cell's construct identity is the
# row's (int_survey_questions only reads construct_* from rows), so a
# column-level tag is silently dropped by the warehouse. Reject loudly
# rather than allow silent data loss — per-column item tagging is a
# future extension if we ever need different items per row dimension.
if element.get("type") == "matrixdropdown":
for column in element.get("columns", []) or []:
if not isinstance(column, dict):
continue
col_label = f"{name}.<col {column.get('name', '?')}>"
for key in ("construct_block", "construct_item"):
if column.get(key) is not None:
raise InvalidDefinition(
f"question {col_label!r}: {key} on a matrixdropdown "
"column is not supported — tag the row instead (a "
"cell's construct identity is its row's)"
)
elif qtype in REPEATING_TYPES:
if isinstance(owner_item, str):
raise InvalidDefinition(
f"question {name!r}: construct_item belongs to a paneldynamic template "
"element, not the panel itself — move it onto the template element"
)
for tmpl in _panel_template_elements(element):
tmpl_label = f"{name}.{tmpl['name']}"
_validate_construct_string(
tmpl_label, "construct_block", tmpl.get("construct_block")
)
_validate_construct_string(tmpl_label, "construct_item", tmpl.get("construct_item"))
tmpl_block = tmpl.get("construct_block") or owner_block
if isinstance(tmpl.get("construct_item"), str) and not isinstance(tmpl_block, str):
raise InvalidDefinition(
f"question {tmpl_label!r}: construct_item set but no construct_block "
"in scope (set it on the template element or the panel)"
)
else:
if isinstance(owner_item, str) and not isinstance(owner_block, str):
raise InvalidDefinition(
f"question {name!r}: construct_item set without construct_block"
)


def _validate_free_text_pii(definition: dict[str, Any]) -> None:
"""Free-text PII gate (invariant 6): pii_risk must be low/high if set, and a
downgrade to 'low' demands an explicit rationale at definition time. Never
Expand Down Expand Up @@ -587,3 +698,4 @@ def validate_definition(definition: dict[str, Any]) -> None:
question_names = _validate_questions(definition)
_validate_visible_if(definition, question_names)
_validate_free_text_pii(definition)
_validate_construct_tags(definition)
194 changes: 194 additions & 0 deletions api/tests/test_publish_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,200 @@ def test_enable_if_dangling_reference_rejected() -> None:
validate_definition(definition)


# --- construct tags ----------------------------------------------------------
# construct_block / construct_item are optional authored *provenance* tags
# (this question is item N of a reusable scale). They are NOT pooling — that
# stays the parent_question_id opt-in (invariant 5). Publish-time validation
# guards shape and the block/item co-occurrence; the dbt construct_pair_integrity
# test backstops the warehouse-side invariant.


def test_construct_tags_on_plain_question_pass() -> None:
validate_definition(_def(_radio("q1", construct_block="phq9", construct_item="phq9_q1")))


def test_construct_block_alone_passes() -> None:
# A block without an item is fine (e.g. "this question is from PHQ-9 but
# I haven't pinned the item id yet"); only construct_item demands a block.
validate_definition(_def(_radio("q1", construct_block="phq9")))


def test_construct_item_without_block_rejected() -> None:
with pytest.raises(InvalidDefinition, match="construct_item set without construct_block"):
validate_definition(_def(_radio("q1", construct_item="phq9_q1")))


def test_construct_block_must_be_string() -> None:
with pytest.raises(InvalidDefinition, match="construct_block must be a non-empty string"):
validate_definition(_def(_radio("q1", construct_block=42)))


def test_construct_item_empty_string_rejected() -> None:
with pytest.raises(InvalidDefinition, match="construct_item must be a non-empty string"):
validate_definition(_def(_radio("q1", construct_block="phq9", construct_item=" ")))


def test_construct_tags_on_matrix_inheritance_pass() -> None:
# Authored canonical pattern for a scale-in-a-matrix: block on the matrix,
# item on each row. Row inherits the matrix's block.
definition = _def(
{
"type": "matrix",
"name": "phq9",
"construct_block": "phq9",
"rows": [
{"value": "q1", "text": "Little interest", "construct_item": "phq9_q1"},
{"value": "q2", "text": "Down or hopeless", "construct_item": "phq9_q2"},
],
"columns": ["0", "1", "2", "3"],
}
)
validate_definition(definition)


def test_construct_item_on_matrix_container_rejected() -> None:
# A matrix groups items; it doesn't *have* an item — that's a row's job.
definition = _def(
{
"type": "matrix",
"name": "m1",
"construct_block": "phq9",
"construct_item": "phq9_q1", # the bug
"rows": ["r1"],
"columns": ["c1"],
}
)
with pytest.raises(InvalidDefinition, match="construct_item belongs to a matrix row"):
validate_definition(definition)


def test_construct_item_on_matrix_row_without_block_rejected() -> None:
# Row carries an item, but neither the row nor the matrix gives a block.
definition = _def(
{
"type": "matrix",
"name": "m1",
"rows": [{"value": "r1", "construct_item": "phq9_q1"}],
"columns": ["c1"],
}
)
with pytest.raises(InvalidDefinition, match="no construct_block in scope"):
validate_definition(definition)


def test_construct_block_on_matrixdropdown_column_rejected() -> None:
# int_survey_questions reads construct_* from rows only — a column-level tag
# would be silently dropped by the warehouse. Reject loudly.
definition = _def(
{
"type": "matrixdropdown",
"name": "md1",
"construct_block": "phq9",
"rows": [{"value": "r1", "construct_item": "phq9_q1"}],
"columns": [
{
"name": "brand",
"cellType": "dropdown",
"choices": ["a", "b"],
"construct_block": "gad7", # the bug
}
],
}
)
with pytest.raises(InvalidDefinition, match="construct_block on a matrixdropdown column"):
validate_definition(definition)


def test_construct_item_on_matrixdropdown_column_rejected() -> None:
definition = _def(
{
"type": "matrixdropdown",
"name": "md1",
"construct_block": "phq9",
"rows": [{"value": "r1"}],
"columns": [
{
"name": "brand",
"cellType": "dropdown",
"choices": ["a", "b"],
"construct_item": "phq9_q1", # the bug
}
],
}
)
with pytest.raises(InvalidDefinition, match="construct_item on a matrixdropdown column"):
validate_definition(definition)


def test_construct_block_on_row_overrides_matrix() -> None:
# A row may carry its own block — the leaf wins over the container.
definition = _def(
{
"type": "matrix",
"name": "m1",
"construct_block": "phq9",
"rows": [{"value": "r1", "construct_block": "gad7", "construct_item": "gad7_q1"}],
"columns": ["c1"],
}
)
validate_definition(definition)


def test_construct_tags_on_paneldynamic_inheritance_pass() -> None:
# Mirror of the matrix story: block on the panel, item on each template cell.
definition = _def(
{
"type": "paneldynamic",
"name": "screener",
"construct_block": "phq9",
"templateElements": [
{
"type": "dropdown",
"name": "q1",
"choices": ["0", "1", "2", "3"],
"construct_item": "phq9_q1",
}
],
}
)
validate_definition(definition)


def test_construct_item_on_panel_container_rejected() -> None:
definition = _def(
{
"type": "paneldynamic",
"name": "screener",
"construct_block": "phq9",
"construct_item": "phq9_q1", # belongs on a template element
"templateElements": [{"type": "dropdown", "name": "q1", "choices": ["a", "b"]}],
}
)
with pytest.raises(
InvalidDefinition, match="construct_item belongs to a paneldynamic template"
):
validate_definition(definition)


def test_construct_item_on_panel_cell_without_block_rejected() -> None:
definition = _def(
{
"type": "paneldynamic",
"name": "screener",
"templateElements": [
{
"type": "dropdown",
"name": "q1",
"choices": ["a", "b"],
"construct_item": "phq9_q1",
}
],
}
)
with pytest.raises(InvalidDefinition, match="no construct_block in scope"):
validate_definition(definition)


# --- endpoint wiring + detail surfacing --------------------------------------


Expand Down
29 changes: 29 additions & 0 deletions dbt/models/intermediate/int_survey_questions.sql
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ scalar_questions as (
-- pii_risk tags free-text questions (design-doc §3.9); null otherwise.
-- The fact gates value_text on this; the API defaults absent → 'high'.
element ->> 'pii_risk' as pii_risk,
-- construct_block / construct_item are optional provenance tags marking a
-- question as belonging to a reusable scale (e.g. 'phq9' / 'phq9_q3').
-- Plain questions read both directly; no inheritance to do here. They are
-- NOT a pooling instruction — cross-version/cross-survey pooling stays the
-- parent_question_id opt-in (invariant 5). Shape is gated at publish; the
-- construct_pair_integrity singular test guards against backdoor inserts.
element ->> 'construct_block' as construct_block,
element ->> 'construct_item' as construct_item,
coalesce(element ->> 'title', stable_name) as prompt_text,
display_order,
-- See the header note: rating/boolean → numeric, text inputType number/range
Expand Down Expand Up @@ -94,6 +102,13 @@ matrix_questions as (
{{ subquestion_name(['e.stable_name', matrix_value('mrow.value')]) }} as stable_name,
e.question_type,
cast(null as text) as pii_risk,
-- construct_block falls back from the row to the matrix (the whole matrix
-- is typically one block, with each row an item within it). construct_item
-- is leaf-only (an item belongs to a row, not the matrix container);
-- validation rejects construct_item set on the matrix element itself.
coalesce(mrow.value ->> 'construct_block', e.element ->> 'construct_block')
as construct_block,
mrow.value ->> 'construct_item' as construct_item,
coalesce(e.element ->> 'title', e.stable_name)
|| ' — '
|| coalesce(mrow.value ->> 'text', {{ matrix_value('mrow.value') }}) as prompt_text,
Expand Down Expand Up @@ -123,6 +138,14 @@ matrixdropdown_questions as (
as stable_name,
e.question_type,
cast(null as text) as pii_risk,
-- See the matrix CTE: block inherits from the matrix; item is leaf-only.
-- A matrixdropdown's leaf is the (row, column) cell, but for v1 the item
-- is tagged at the row level (every column of one row shares the item —
-- "rate PHQ-9 item 3 on these two dimensions"); per-column item is a
-- future extension if needed.
coalesce(mrow.value ->> 'construct_block', e.element ->> 'construct_block')
as construct_block,
mrow.value ->> 'construct_item' as construct_item,
coalesce(e.element ->> 'title', e.stable_name)
|| ' — '
|| coalesce(mrow.value ->> 'text', {{ matrix_value('mrow.value') }})
Expand Down Expand Up @@ -158,6 +181,12 @@ paneldynamic_questions as (
{{ subquestion_name(['e.stable_name', "tmpl.value ->> 'name'"]) }} as stable_name,
tmpl.value ->> 'type' as question_type,
tmpl.value ->> 'pii_risk' as pii_risk,
-- Block inherits from the panel container; item is leaf-only (a template
-- element is the item-bearing unit, not the panel itself). Mirror of the
-- matrix rule — validation rejects construct_item on the panel itself.
coalesce(tmpl.value ->> 'construct_block', e.element ->> 'construct_block')
as construct_block,
tmpl.value ->> 'construct_item' as construct_item,
coalesce(e.element ->> 'title', e.stable_name)
|| ' — '
|| coalesce(tmpl.value ->> 'title', tmpl.value ->> 'name') as prompt_text,
Expand Down
Loading
Loading