Issue #1011: harden GraphQL schema build against invalid source names by bjagg · Pull Request #1012 · LIF-Initiative/lif-core

bjagg · 2026-06-23T21:19:51Z

Description of Change

Problem. A single MDR field whose name contains an illegal GraphQL character — the concrete case is the hyphen in iSO639-2LangCode — makes the entire Strawberry/GraphQL schema build raise GraphQLError: Names must only contain [_a-zA-Z0-9]…. graphql-org1 crashes on startup (exit 3), ECS crash-loops it (0/1), and the ALB returns 503 for everything. (This is the concrete instance behind spike #369; surfaced while verifying the LDE dev deploy, #998.)

Root cause. openapi_to_graphql/type_factory.py exposed the raw source-schema name as the GraphQL name (strawberry.field(name=field_name), type/enum names), so one bad name fails the whole schema build.

Fix. Add safe_graphql_name() to string_utils — a case-preserving sanitizer (distinct from safe_identifier, which snake_cases for Python attrs). It only replaces invalid characters with _ and prefixes _ on a leading digit, so valid names are unchanged (zero behavior change for healthy schemas). Apply it at every GraphQL-name boundary in type_factory.py:

exposed field names — object types and both input/filter builders (9 sites)
object/array sub-type names (create_type entry — also the cache key, so it's consistent)
enum type names (create_enum_type entry)
input/filter type names (both builders' entry)

Net effect: an invalid source name is sanitized rather than fatal. iSO639-2LangCode is exposed as iSO639_2LangCode (queryable) instead of taking the service down.

Side effects / limitations. None for valid schemas (sanitizer is identity on valid names). For an invalid name, the exposed GraphQL field name changes (to the sanitized form) — but such a name was previously un-queryable (it crashed the service), so this is strictly an improvement. Data resolution is unaffected (it goes through the snake_cased Python attribute, not the exposed name).

How reviewers should test. uv run pytest test/components/lif/string_utils/ test/components/lif/openapi_to_graphql/ — adds a safe_graphql_name unit test and a regression test that generate_graphql_schema builds successfully with a hyphenated field and exposes iSO639_2LangCode. The regression test fails pre-fix with the exact GraphQLError.

Related Issues

Closes #1011 · likely resolves spike #369 · surfaced during #998

Type of Change

Bug fix (non-breaking change which fixes an issue)

Project Area(s) Affected

components/
test/ or e2e/
API endpoints (GraphQL schema generation)

Checklist

commit message follows commit guidelines
tests are included (unit + regression)
code passes linting checks (uv run ruff check)
code passes formatting checks (uv run ruff format)
code passes type checking (uv run ty check) — no new diagnostics (23→23)
pre-commit hooks have been run successfully

Testing

Automated tests added/updated (red→green verified against pre-fix code)

Additional Notes

Also fixes two pre-existing lint nits in the touched test file (unused List import, redundant == str) so the re-staged file passes ruff.
Recovery: once merged and graphql-org1 redeploys, it recovers automatically (schema build succeeds). The interim data workaround (renaming the MDR element) is no longer required after this lands, though it remains a valid immediate unblock.

🤖 Generated with Claude Code

…d source names A single MDR field named with an illegal GraphQL character (e.g. the hyphen in 'iSO639-2LangCode') made the entire Strawberry/GraphQL schema build raise GraphQLError ("Names must only contain [_a-zA-Z0-9]") and crash graphql-org1 on startup, crash-looping the service (0/1, 503). The generator exposed the raw source name as the GraphQL name. Add a case-preserving sanitizer, safe_graphql_name() (distinct from safe_identifier, which snake_cases for Python attrs): it only replaces invalid characters with '_' and prefixes '_' on a leading digit, so valid names are unchanged. Apply it at every GraphQL-name boundary in type_factory.py — exposed field names (object + both input/filter builders), object/array sub-type names (create_type entry, used as the cache key too), enum type names, and input type names. Net effect: an invalid source name is sanitized rather than fatal. Add a unit test for safe_graphql_name and a regression test that generate_graphql_schema builds successfully with a hyphenated field and exposes it as iSO639_2LangCode (fails pre-fix with the exact GraphQLError). Also fixes two pre-existing lint nits in the touched test file (unused import, redundant `== str`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…eserved "__", collisions) Follow-up to the initial hardening, closing two gaps found while auditing the broader "permissive source names vs. strict consumers" class: - safe_graphql_name now collapses a leading run of underscores to one, so a sanitized name can't begin with "__" (GraphQL reserves "__" for introspection → would still crash the build). - create_type de-dups field names: two source names that collapse to the same identifier no longer produce a duplicate GraphQL field (build crash) or a silently overwritten Python attribute — the later one is suffixed and a warning is logged. Adds unit + schema-build regression tests for both. Residual (intentionally deferred to MDR write-time validation, LIF-Initiative#1014): type-name collisions and reserved type names (String/Query/etc.). The durable fix is to reject/normalize invalid names at the MDR boundary rather than sanitize in every consumer; this commit keeps the GraphQL generator from crashing in the meantime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ing + audit evidence Frame the problem around LIF's two entry points (MDR schema/mapping definitions, which can be guarded; vs. data, which is validated against its assigned schema + sanity checks). Scope this ADR to the MDR entry point and note the data-vs-type conformance sibling concern (LIF-Initiative#1017). Add the audit evidence that the permissive-name problem is not GraphQL-specific: the same illegal name independently threatens GraphQL (LIF-Initiative#1011/LIF-Initiative#1012), semantic-search/MCP (LIF-Initiative#1016), MongoDB ($/. keys), the composer dot-path split, and JSONata — each sanitizing differently or not at all, which is the core argument for validating once at the MDR boundary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…uilds Promotes the 26 demo service param files to the images currently running in dev (via scripts/release-demo.sh): most -> 0025436 (06-19 build), advisor-app -> 66cd748 (#1003), learner-data-export-api -> 43b2084 (#1010). graphql bumps 05-16 -> 06-19 (note: demo graphql-org1 stays down on #1011 regardless — that's data-driven, fixed by #1012 + redeploy or the MDR rename, not this image bump). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…uilds (#1020) ##### Description of Change Promotes the **26 demo service param files** to the images currently running in dev, via `scripts/release-demo.sh --apply` (resolves each repo's dev `:latest` to its pinned version tag). Tag-only changes (26 removed / 26 added; verified no structural edits). Notable pins: - most services → `2026-06-19-…-0025436` (dev's 06-19 build) - `advisor-app` → `2026-06-23-…-66cd748` (the #1003 lockfile fix) - `learner-data-export-api` → `2026-06-23-…-43b2084` (#1010) - `graphql-org{1,2,3}` → 06-19 build (note: demo graphql stays down on #1011 regardless — data-driven, fixed by #1012 + redeploy or the MDR rename, not this image bump) **Deploy:** the 22 real service stacks (∩ `STACK_ORDER`, in order) are being deployed per-stack via `aws-deploy.sh -s demo --only-stack …`. **LDE is excluded** — the demo LDE stack/SSM keys don't exist yet (that's #999). The bare `demo-lif-{graphql,query-cache,query-planner}.params` are shared/template params not in `STACK_ORDER`, so they're updated but not separately deployed. ##### Related Issues Demo promotion (dev → demo). Refs #999 (LDE-to-demo, deferred), #1011/#1012 (demo graphql still down until fixed). ##### Type of Change - [x] Infrastructure/deployment change ##### Project Area(s) Affected - [x] cloudformation/ or sam/ templates --- ##### Checklist - [x] commit message follows commit guidelines ##### Additional Notes Generated by `release-demo.sh`; deploy run separately (per-stack, LDE excluded). MDR frontend (S3/CloudFront) and SAM databases are separate guide steps, not included here. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

…uilds (#1023) ## What Promotes 8 demo services to dev build \`328d493\` (the #1022 merge build). #1022 (available-data-formats) changed shared MDR components (\`mdr_client\` / \`mdr_services\` / \`mdr_dto\`), so the monorepo CI rebuilt every service that depends on them. Promoting them together keeps the new LDE \`/available-data-formats\` endpoint consistent with the new MDR \`exportable\` param. | Stack | From → To | |-------|-----------| | graphql-org1/2/3 (+ base) | 0025436 → 328d493 | | learner-data-export-api | 43b2084 → 328d493 | | mdr-api | 0025436 → 328d493 | | semantic-search | 0025436 → 328d493 | | translator-org1 | 0025436 → 328d493 | The other 26 services don't depend on the changed components → unchanged (`release-demo` reports them already current). ## Notes - graphql image is pre-#1012; safe because the demo MDR field rename (attrs 1404/1405/1406, hyphen→underscore) is in place (verified pre-deploy). - Does not address the demo `/exports` ELM/HR-Open transformation-mapping content gap (separate). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cbeach47

Static review looks good.

1 nit on the comment inline.

Overall - how does this behave when there are unsafe entity / attribute names in the Semantic Search, Query Planner, Orchestrator, LDE flows? The names are sanitized, but then there is a mismatch to the sanitized name and the persisted name?

cbeach47 · 2026-06-30T22:24:21Z

+def safe_graphql_name(name: str) -> str:
+    """Sanitize a string into a valid GraphQL name, preserving its original casing.
+
+    GraphQL names must match /[_A-Za-z][_0-9A-Za-z]*/ — only ``[_0-9A-Za-z]`` and not


The wording is a bit unclear. I think what it's saying is:

GraphQL names must be at least 1 character long, starting with _A-Za-z, and only contain _0-9A-Za-z.

…ders via shared helper Review follow-up: the field-name sanitization de-dup lived only in create_type, so a sanitization collision among queryable/mutable fields could still silently drop a field (dict-key overwrite) or risk a duplicate-field crash in the filter/ mutation input types. Extract the de-dup into a shared resolve_field_names() helper and apply it in create_type, create_mutable_input_type, and create_nested_input_type so all GraphQL types and inputs harden identically. Add a regression test asserting colliding queryable names both survive in the generated filter input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

bjagg · 2026-06-30T23:11:57Z

@cbeach47 — ready for review. Durable fix for the #1011 GraphQL schema-build crash on invalid source names (e.g. iSO639-2LangCode). Just pushed a follow-up (50a4919) extracting the field-name de-dup into a shared resolve_field_names() helper so the filter/mutation input builders harden the same way as create_type — with a regression test. Tests green.

cbeach47 · 2026-06-30T23:45:00Z

Actually, partway through the review of #1015 . We may want to align on that ADR, and the downstream effects of sanitization of entities / attributes before we ship the fix.

…w nit) Reword the GraphQL-name-rule sentence per cbeach47's review — state the rule plainly (>=1 char, starts with _A-Za-z, otherwise only _0-9A-Za-z) rather than leading with the regex. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

bjagg · 2026-07-02T22:12:05Z

Traced the field-name path end to end — you're right, and here's exactly where it diverges (for the invalid-named fields only):

GraphQL → Query Planner (confirmed mismatch). The root resolver builds selected_fields from the GraphQL AST (get_selected_field_paths → selection.name.value) — i.e. the exposed (sanitized) name — and posts that straight to the QP; there's no reverse map back to the source name. The QP matches selected_fields against the MDR schema by exact string, which holds the source name (iSO639-2LangCode). So a query for the sanitized iSO639_2LangCode finds nothing → that field comes back null. Confirmed: sanitized fields don't round-trip.

Semantic search (separate, pre-existing). It builds its GraphQL query from the MDR json_paths (source names, un-sanitized), so it emits iSO639-2LangCode — which fails GraphQL validation against the sanitized schema. That's its own inconsistency, already tracked as #1016 (semantic-search invalid-name handling, parallel to #1011).

LDE. Unaffected — it bypasses GraphQL and calls the QP directly with hardcoded source LIF paths (Person.Name, …), so no sanitization is in play. Round-trips fine.

What this means for #1012. Scope matters: safe_graphql_name is a no-op for every valid name, so the divergence is confined to genuinely-invalid fields — today just the three CEDS iSO639-2/3/5 fields. And the baseline it replaces is worse: those names currently crash the entire schema build and take graphql-org1 down — every field, every org. So #1012 turns "whole service down" into "service up; the 3 invalid-named fields return null." It's a strict improvement and a crash guard — but you're right that it does not make those fields resolve.

The fix that makes them resolve is the ADR direction, not consumer-side plumbing. I specifically don't want to add a sanitized→source reverse map into the resolver (then the QP, then semantic search…) — that's exactly the per-consumer sprawl #1015 argues against. The clean fix is normalize-at-the-MDR-boundary: #1013 renames those three fields in the source (schema + migration + live data) so the persisted name is already GraphQL-valid. After #1013 the GraphQL name == the source name, all four flows round-trip, and #1012 never fires on real data.

Proposed sequencing: ship #1012 now as the crash guard (keeps graphql up regardless of MDR contents), ratify #1015 as the policy, and prioritize #1013 to fix those fields at the source (plus #1016 for the semantic-search side). That de-risks the outage immediately without blocking on the data cleanup, while committing to the cleanup as the real resolution. Does that sequencing work for you?

(Also pushed the docstring reword you flagged — 2d2e8e8.)

bjagg · 2026-07-08T23:48:54Z

@cbeach47 — this one's waiting on your call on the sequencing. My reply above traces the sanitized-vs-persisted name mismatch end-to-end through the QP/LDE/semantic-search flows and confirms your concern. Proposed path: ship #1012 now as the crash-guard (service-up beats whole-schema-crash) and fix at the source via #1013 (rename the CEDS fields) + ratify #1015 (normalize-at-MDR-intake) — not consumer-side reverse-mapping. Agreeing this also unblocks #1015 (Tammie's already 👍'd it). Does that sequencing work for you?

…ization Records the conflict between source-standard field names (CEDS et al.), the LIF naming convention, and consumer technology constraints (GraphQL/Python/JSON) — the class of problem behind the iSO639-2LangCode GraphQL crash (#1011). Status Proposed: captures context, options, and a recommended decision (normalize on MDR intake + preserve source name as metadata + write-time enforcement, with the #1012 codegen sanitization as defense-in-depth) for the team to ratify or revise. Adds the ADR to docs/INDEX.md and drops data_model/ from the template-only note. Refs #1014 (ADR + non-tech guide + enforcement), #1011/#1012, #1013. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ion (#1015) ##### Description of Change Drafts **ADR 0001 (Proposed)** under `docs/design/adr/data_model/` capturing a conflict that has surfaced repeatedly but was never recorded as a decision: **source-standard field names** (CEDS et al.) vs. the **LIF naming convention** vs. **consumer technology constraints** (GraphQL's `/[_A-Za-z][_0-9A-Za-z]*/`, Python snake_case, JSON/Mongo verbatim, the translator's case-insensitive lookups). This is the class of problem behind the `iSO639-2LangCode` GraphQL crash (#1011/#1012). The ADR documents the context, the alternatives, and a **recommended** decision — normalize names on MDR intake to a canonical LIF name, preserve the original source name as provenance metadata ("no loss"), enforce at the MDR write boundary, and keep the #1012 codegen sanitization as defense-in-depth — explicitly marked **Proposed** for the team to ratify or revise. Also adds the ADR to `docs/INDEX.md` and removes `data_model/` from the "template-only placeholders" note. **This is a draft for discussion — it should not be merged as `Accepted` until the team ratifies the decision** (the status line says as much). Merging it as `Proposed` to make it visible is also fine; the team can flip the status in a follow-up. ##### Related Issues Part of #1014 · refs #1011, #1012, #1013 ##### Type of Change - [x] Documentation update ##### Project Area(s) Affected - [x] Documentation (docs/, READMEs, ARCHITECTURE.md, CLAUDE.md) --- ##### Checklist - [x] commit message follows commit guidelines - [x] documentation is changed or added (in /docs directory) ##### Additional Notes Follows the repo ADR template (Status / Context / Decision / Alternatives / Consequences / References). The non-technical naming guide and enforcement/lint pieces from #1014 are intentionally **not** in this PR — this is just the decision record. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

bjagg · 2026-07-14T23:45:53Z

👋 @dereck-symmetry — this one's yours now.

What it is: hardens the GraphQL schema build (#1011) so a single invalid source field name (e.g. a hyphen — iSO639-2LangCode) can't crash the whole schema build and take the service down; it sanitizes names to valid GraphQL and de-dups collisions.

Where to look: components/lif/string_utils/core.py (safe_graphql_name) + components/lif/openapi_to_graphql/type_factory.py (applied at every name boundary + a shared de-dup helper) + the tests.

Context: there was an open design question (does the sanitized name still resolve to the persisted source name?) — it's now settled by ADR 0001 (#1015, merged): normalize at the MDR boundary (#1013), with this PR as the crash-guard. So there's no open decision here — it's a review of the guard + tests. Claude-authored; focus on reasoning + coverage.

dereck-symmetry

LLM helped in review.
Creation of the new 'safe_graphql_name' plus the unit tests is a correct direction.
The Regex patterns are correct (matches GraphQL) and allow correct matching of the incoming. Empty string pass-through is a good compromise because OpenAPI wouldn't pass an empty string into the system.
Per direction of the issue, all five entry points have been hardened by updates to the code here.
Unit tests are clear and readable - and test all of the things updated.
LLM does note that a bare tuple[str,str] is used, and that this return type would only be legal in Python 3.13 and above. But the .toml was checked and the project is constraining the Python to between 3.13 and 3.14.
test_core.py tests are clear and shown to de-dup and correctly set the names.
Approve.

github-project-automation Bot added this to Current Community Efforts Jun 23, 2026

github-project-automation Bot moved this to Backlog in Current Community Efforts Jun 23, 2026

bjagg requested a review from cbeach47 June 23, 2026 22:50

This was referenced Jun 24, 2026

Semantic-search/MCP: dynamic filter & mutation models crash on invalid source field names (parallel to #1011) #1016

Open

GraphQL resolver: no int/bool/float coercion — typed fields silently null (or mismatch) for string source data #1017

Open

bjagg mentioned this pull request Jun 29, 2026

chore: release-demo 2026-06-29 — pin demo image tags to current dev builds #1023

Merged

cbeach47 reviewed Jun 30, 2026

View reviewed changes

bjagg requested a review from dereck-symmetry July 14, 2026 23:44

dereck-symmetry approved these changes Jul 18, 2026

View reviewed changes

bjagg merged commit 450ab91 into LIF-Initiative:main Jul 19, 2026

github-project-automation Bot moved this from Backlog to Done in Current Community Efforts Jul 19, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Issue #1011: harden GraphQL schema build against invalid source names#1012

Issue #1011: harden GraphQL schema build against invalid source names#1012
bjagg merged 4 commits into
LIF-Initiative:mainfrom
bjagg:fix/issue-1011-graphql-name-hardening

bjagg commented Jun 23, 2026

Uh oh!

cbeach47 left a comment

Uh oh!

cbeach47 Jun 30, 2026

Uh oh!

bjagg commented Jun 30, 2026

Uh oh!

cbeach47 commented Jun 30, 2026

Uh oh!

bjagg commented Jul 2, 2026

Uh oh!

bjagg commented Jul 8, 2026

Uh oh!

bjagg commented Jul 14, 2026

Uh oh!

dereck-symmetry left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

bjagg commented Jun 23, 2026

Description of Change

Related Issues

Type of Change

Project Area(s) Affected

Checklist

Testing

Additional Notes

Uh oh!

cbeach47 left a comment

Choose a reason for hiding this comment

Uh oh!

cbeach47 Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

bjagg commented Jun 30, 2026

Uh oh!

cbeach47 commented Jun 30, 2026

Uh oh!

bjagg commented Jul 2, 2026

Uh oh!

bjagg commented Jul 8, 2026

Uh oh!

bjagg commented Jul 14, 2026

Uh oh!

dereck-symmetry left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants