fix(openapi): resolve $refs under patternProperties for map schemas

[acrylJonny]

Summary

• OpenAPI schema resolution previously skipped patternProperties, leaving #/components/schemas/... refs unresolved so map-like response bodies ingested with no columns.
• Resolve refs under patternProperties (and propertyNames, so leftover $refs cannot break jsonref), merge allOf map/validation keywords safely, and promote to additionalProperties when needed so json_schema_util can extract fields.
• Harden the source: per-endpoint try/except, request timeouts, non-fatal 401/429 handling, typed get_token / coerced forced_examples, no vestigial catch_warnings crash path, safe OpenAPI prerelease version parsing, and report-visible schema conversion failures (swallow_exceptions=False).

Test plan

• Unit tests for patternProperties / allOf merge / promotion / propertyNames $ref / prerelease version / per-endpoint isolation / forced_examples / get_token
• ./gradlew :metadata-ingestion:lint and :metadata-ingestion:testSingle -PtestFile=tests/unit/test_openapi.py

Follow-ups (out of scope)

• Emit dataPlatformInstance aspect (standards gap; pre-existing)

[cursor]

PR Summary

Overview
Fixes OpenAPI/JSON Schema ingestion so map-like response fields and allOf compositions produce extractable columns instead of empty or broken schemas.

OpenAPI resolution (openapi_parser) now walks patternProperties and propertyNames for $ref resolution, and for map-only objects promotes patternProperties to additionalProperties (single pattern or anyOf when multiple) so downstream extraction can read map value types. allOf enum values are intersected across members rather than first-wins; when intersection is empty, enum is removed so invalid enum: [] does not fail whole-schema validation and drop unrelated fields.

JSON schema typing (json_schema_util) adds _is_map_schema: a schema is treated as a map only when additionalProperties is an object schema and there are no named properties. Typeless maps (only additionalProperties, no type) now resolve as maps with value types extracted; objects that mix named properties with additionalProperties stay records so nested fields are not lost.

^{Reviewed by Cursor Bugbot for commit 5b3089d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[codecov]

Codecov Report

❌ Patch coverage is 91.30435% with 4 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ion/src/datahub/ingestion/source/openapi_parser.py	91.42%	3 Missing ⚠️
...rc/datahub/ingestion/extractor/json_schema_util.py	90.90%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[cubic-dev-ai]

All reported issues were addressed across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[acrylJonny]

Addressed the Bugbot / cubic review notes:

• Only promote patternProperties → additionalProperties when both properties and additionalProperties are absent
• Run promotion after allOf merge so properties contributed by allOf are preserved
• Added regression tests for named+pattern and allOf+pattern cases

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cursor]

Stale Bugbot comment from a previous run.

[cubic-dev-ai]

All reported issues were addressed across 5 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 1 file (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cursor]

Stale Bugbot comment from a previous run.

[max-datahub]

Review

Thanks for digging into the map-schema extraction gap — the core fix is correct and the malformed-input hardening comes from a real place. My main feedback is about scope: the titled bug fix is a small fraction of a ~1,770-line source rewrite (+3,900 lines of tests) that bundles several unrelated concerns, and a good chunk of the added complexity has no effect on what this connector actually emits. I'd like to see it split before merge. Details below, grouped by structural concerns → correctness nits → tests, with the good parts first.

What's working well

• The titled fix is right. Resolving $refs under patternProperties/propertyNames and promoting map-only schemas to additionalProperties, plus the json_schema_util list-form-type map handling (type: ["object","null"] → map), is the correct shape for the "map bodies ingest with no columns" bug.
• Removing warnings.catch_warnings is a genuine fix, not just a refactor. The parser emits zero warnings.warn calls, so the old block only caught incidental library warnings and then did w.message.args[0].split(" --- ")[1] — a latent IndexError on any warning without that separator. Good to see it gone.
• get_tok credential-leak hardening is careful and correct. raise ... from None + status-only messages genuinely prevent the password-in-URL (GET token flow) from surfacing through report.failure's verbatim exception rendering. This deserves to land.
• Request timeouts (30s), the shared-sw_dict mutation-safety copies (avoiding cross-endpoint corruption), and switching report_bad_responses off raise-on-unmapped-code to graceful degradation are all correct.

Structural concerns

1. Scope / reviewability. The PR bundles at least four independent changes under a fix(openapi): resolve $refs under patternProperties title: (a) the map/$ref fix, (b) an allOf-merge engine rewrite, (c) a broad hardening sweep (auth typing, SecretStr truthiness semantics, timeouts, get_token/forced_examples validation), and (d) the parser→report warning bridge. Each carries independent regression risk for a widely-used connector, and a bug in any of them ships under an innocuous title with no clean way to bisect. Recommend splitting — at minimum peel the get_tok security fix and the report bridge into their own PRs; the actual reported bug is fixable in a small fraction of this diff.

2. The numeric/validation allOf-merge logic (~130 lines) has no effect on connector output. _merge_allof_validation_keywords, _merge_bound_with_exclusivity, _merge_exclusive_bound, _apply_bool_exclusivity, _merge_numeric_keyword, _ALLOF_NUMERIC_BOUNDS merge minimum/maximum/exclusive*/minLength/maxItems/multipleOf/uniqueItems. The sole consumer of this resolution path, json_schema_util.get_schema_metadata, reads none of these keywords — it emits fields from type/properties/additionalProperties/items/oneOf/anyOf/allOf/enum only. So all of this intricate OpenAPI-3.0-boolean-vs-draft-6-numeric exclusivity handling changes only the cosmetic platformSchema.rawSchema blob, never the navigable fields. That's a large, edge-case-dense surface to maintain for zero observable behavior. Suggest dropping it and keeping only the merges that feed extraction (properties/required/items/additionalProperties/patternProperties/oneOf/anyOf).

3. The parser→report warning bridge (~70 lines) is a lot of concurrency machinery, and has a correctness gap. _RefcountedLevelOverride (lock + refcount + saved-level) plus the per-thread-filtered _CollectingLogHandler defend against multiple APISource runs sharing the module logger concurrently — which the sequential ingestion pipeline doesn't do. More importantly, _CollectingLogHandler.emit binds to the thread that first advances the get_workunits_internal generator; if a consumer ever resumes that generator on a different thread, every parser warning is silently dropped from the report — the exact bridging this code exists to provide. Simpler options, cheapest first: rely on the module logger like every other free function in openapi_parser already does, or give the handful of parser functions an optional collector/report argument.

Correctness inconsistencies (lower blast radius)

• json_schema_util._get_type_from_schema — the two map-detection branches diverge. The new list-form branch gates on and not schema.get("properties"); the existing string-form branch (type: "object") does not. So a hybrid schema with both properties and dict additionalProperties is classified "map" when type: "object" (named fields dropped) but "object" when type: ["object","null"] (map value dropped) — same logical schema, opposite output depending only on nullability. The inline comment claiming it does "the same map check as the plain-string-type branch below" is inaccurate. Please align the two branches and fix the comment.
• _ALLOF_FIRST_WINS_KEYWORDS applies first-wins to type and enum. Under allOf, enum should intersect ([1,2,3] ∩ [2,3,4] = [2,3]), not take the first member. Descriptive-only here so low impact, but semantically wrong for a legal composition.
• stringify_forced_examples coerces bool via str() → "True"/"False". The field doc says "bool is accepted via int" (implying "1"), but str(True) == "True", which most APIs expecting a boolean path param will reject. It's also untested (the added tests cover only int→"1" and null-rejection).
• schema_resolution_max_depth gains ge=1, le=100 — a config-validation tightening that rejects previously-accepted recipes (0, or >100) but isn't listed in the updating-datahub.md breaking-changes entry (which correctly documents the get_token/forced_examples changes). Low severity, but please document it or relax the bound.
• Double warning on schema-extraction failure. In _process_endpoint, when the extraction try raises, the except emits "Schema Extraction Failed" and sets schema_metadata=None; the subsequent falsy-schema branch then also increments no_schema_found and emits "No Schema Extracted" for the same endpoint — two slightly contradictory report entries for one failure.

Test suite

There are ~187 new test functions here, and while the core ones are strong, the suite is inflated in three ways that mirror the source concerns:

• ~13 test dead or speculative code. The ~11 numeric-bound merge tests assert on minLength/maximum/exclusiveMaximum values that json_schema_util never reads (test_merge_allof_integer_bounds_preserve_int_type literally asserts max(3,5) returns an int — a language feature). The two _capture_parser_warnings thread tests exist only to cover the log bridge above. If concerns #2 and #3 are addressed, these delete for free.
• ~45 are near-duplicates that should be parametrized (→ ~12). The clearest: the 9 test_get_endpoints_malformed_*_does_not_abort_other_endpoints tests are byte-identical except for where the malformed value sits (@pytest.mark.parametrize over (spec, expected)); the 7 bool-subschema-collision tests exercise the same _intersect_subschemas semantics across three keyword slots; the 8 normalize_bare_boolean_*/normalize_none_* tests apply the same rule per slot; the 4 check_sw_version_* tests vary only the version string.
• A handful are trivial plumbing — e.g. test_request_call_forwards_proxies_on_* mock-assert that a kwarg reaches requests.get, and test_get_swag_json_parses_json_response covers the happy path of json.loads.

The genuinely valuable tests — mutation-safety (*_does_not_mutate_shared_component*), credential-leak prevention, circular-ref/depth-cap termination, oneOf/anyOf sibling preservation, per-endpoint isolation, and the core patternProperties resolution — are excellent and should stay. A focused ~55–70 tests would give equivalent regression protection.

(Minor style note, for the file rather than this PR: metadata-ingestion's AGENTS.md asks for pytest + bare assert rather than unittest.TestCase/self.assertEqual; the new tests match the existing TestCase classes in the file, which is a reasonable consistency call.)

Suggested path forward

1. Land the map/$ref fix (+ the json_schema_util branch alignment) as a small, focused PR.
2. Land the get_tok credential-leak fix separately.
3. Drop the numeric-bound merge engine and the threaded log bridge.
4. Fold the malformed-input hardening in on its own, with the near-duplicate tests parametrized.

Happy to look again once it's split — the underlying fix is solid and worth getting in.

• Claude (on behalf of Max)

[acrylJonny]

Thanks @max-datahub — really thorough review. Latest push (1eb10fe) addresses the concrete correctness items:

• json_schema_util map-branch divergence — the string-form (type: object) and list-form (type: [object, null]) branches now share a single _is_map_schema helper that requires no named properties before returning "map", so the hybrid properties + dict additionalProperties case is classified identically regardless of nullability. Fixed the misleading inline comment too. This also fixed a Bugbot finding on the current HEAD: typeless additionalProperties maps (no type) were resolving to an empty object and dropping their value type — _is_map_schema now covers the typeless case as well.
• enum under allOf — now intersects members' allowed values instead of first-wins ([1,2,3] ∩ [2,3,4] → [2,3]); removed enum from _ALLOF_FIRST_WINS_KEYWORDS.
• stringify_forced_examples bool coercion — True/False now become "1"/"0" (bool checked before int) to match the documented "bool via int", instead of str(True) == "True".
• schema_resolution_max_depth 1..100 bound — now documented in the updating-datahub.md fix(openapi): resolve $refs under patternProperties for map schemas #19383 entry.
• Double warning on extraction failure — _process_endpoint no longer emits a second "No Schema Extracted" after it already reported "Schema Extraction Failed" for the same endpoint.
• items: false (separate Bugbot finding) — non-dict items now normalizes to {} rather than being dropped, so an array is never silently mistyped as an array of strings.

Each has a focused unit test.

On the structural feedback (split the PR; drop the numeric-bound allOf merge engine and the threaded parser→report log bridge): I hear you and I don't disagree that the map/$ref fix is the small core of a large diff. I've left the split + those two removals out of this push since they're a bigger call than the correctness fixes — happy to go whichever way you prefer:

1. carve this down to just the map/$ref fix (+ json_schema_util alignment) and peel get_tok and the report bridge into their own PRs, or
2. drop the numeric-bound merge engine + threaded bridge here and keep the rest.

Let me know which and I'll turn it around.

[max-datahub]

Thanks for the fast turnaround @acrylJonny — I went through 1eb10fe and the six correctness items all check out:

• _is_map_schema unifies both branches, so the hybrid properties + dict additionalProperties case is now classified consistently regardless of nullability (and the misleading comment is gone) ✅
• enum intersects across allOf members, and the list-membership test avoids the unhashable-value trap ✅
• bool forced_examples → "1"/"0" ✅
• schema_resolution_max_depth 1..100 documented ✅
• no second "No Schema Extracted" after a "Schema Extraction Failed" for the same endpoint ✅
• items: false → {} with the intentional (and correctly reasoned) asymmetry vs properties/patternProperties ✅

One small leftover: type is still in _ALLOF_FIRST_WINS_KEYWORDS — the original note covered type as well as enum. Low priority (a genuine type conflict across allOf members is malformed input), so fine to leave — just flagging it's half-addressed.

One thing worth surfacing before merge: the json_schema_util._get_type_from_schema change isn't openapi-scoped. JsonSchemaTranslator is a shared extractor used by ~20 connectors (kafka / confluent / kinesis schema registry, the json-schema source, glue, salesforce, pulsar, dbt, bigquery, tableau, oracle/vertica/hive, and the schema transformers). Two behavior changes ride along for all of them:

1. On master, type: "object" + dict additionalProperties resolves to "map" regardless of properties; with _is_map_schema now requiring no properties, a schema declaring both flips "map" → "object".
2. A typeless additionalProperties map now resolves to "map" (was "object").

Both look like correctness improvements to me, but they change a widely-shared module under a fix(openapi) title and are only covered by the openapi / json_schema_util unit tests. Could you run the affected connectors' golden/integration tests (kafka + json-schema at minimum) to confirm no golden-file drift — or gate the new map detection behind the openapi path if we'd rather not touch shared behavior in this PR?

On the structural choice: I'd go with Option 1 — carve this down to the map/$ref fix + the json_schema_util alignment, and peel get_tok and the report bridge into their own PRs. The shared-extractor change above is exactly why the core wants the smallest, most-reviewable PR with its own test scrutiny, and isolating get_tok keeps the security fix easy to audit and backport. One ask for the follow-up: when the hardening PR lands, please delete the numeric-bound allOf merge engine and the threaded parser→report bridge rather than relocating them — json_schema_util reads none of the numeric keywords, so that logic has no effect on extracted fields.

Thanks again — the core fix is solid and I'm keen to see it land.

• Claude (on behalf of Max)

[acrylJonny]

Thanks @max-datahub — good call flagging the shared-module blast radius. I confirmed against master:

• String-form type: "object": master returned "map" whenever additionalProperties was a dict regardless of properties, so a hybrid properties + dict-additionalProperties schema was classified map and its named properties were dropped. _is_map_schema now gates on no properties, so it resolves to object — a strict correctness improvement, not a regression.
• Typeless additionalProperties now resolves to map (was object), extracting the value type.

No golden-file drift. I ran every source that imports JsonSchemaTranslator:

Importer	Tests	Result
json-schema source	tests/unit/schema/	71 passed
kinesis schema registry	tests/unit/kinesis/	117 passed
kafka + confluent schema registry	tests/unit/kafka/, tests/unit/confluent/	247 passed

Worth noting the kafka integration golden fixtures (kafka_mces_golden.json et al.) are Avro-only (*.avsc), so they route through avro_util, not JsonSchemaTranslator — they can't drift from this change. The JSON-schema path is exercised by the unit suites above.

On the type first-wins note: agreed it's fine to leave — a genuine type conflict across allOf members is malformed input, and first-wins matches the pre-existing behavior for the other unmergeable keywords.

On the structural split (Option 1) + deleting the numeric-bound allOf engine and the parser→report bridge in the follow-up: acking that. I'll confirm the exact split plan and follow up separately rather than reshuffling under this thread.

[acrylJonny]

Went with Option 1 and carved this PR down to the core, pushed in decb902. The PR now touches 4 files, +195/-7 against master:

• openapi_parser.py — resolve $refs under patternProperties/propertyNames, promote map-only patternProperties schemas to additionalProperties (after allOf merge, so named properties win), and intersect enum across allOf members instead of first-wins.
• json_schema_util.py — the _is_map_schema unification across the string-form / list-form / typeless branches.
• Focused unit tests for both.

Everything else is peeled out and preserved on branch openapi-full-hardening-snapshot for follow-up PRs: the typed get_token config, forced_examples validation, the schema_resolution_max_depth bound, the numeric-bound allOf merge engine, the parser→report bridge, and the broader malformed-input hardening. When the hardening PR lands I'll delete the numeric-bound allOf engine and the threaded report bridge rather than relocate them, as you asked (json_schema_util reads none of the numeric keywords, so they don't affect emitted fields).

To your shared-extractor concern: I ran every source that imports JsonSchemaTranslator on the reduced branch — json-schema (tests/unit/schema/), kinesis, kafka, confluent — all green, no golden drift.

The type-still-first-wins nit: leaving as-is per your OK (a real type conflict across allOf members is malformed input).

[acrylJonny]

Follow-ups are up, stacked so each shows only its own diff:

1. feat(ingest/openapi): typed get_token config + credential-leak hardening #19905 — typed get_token config + credential-leak hardening in get_tok (base: this PR's branch). Isolated so the security-relevant get_tok change is easy to audit/backport: sanitized exception messages (no password-substituted URL or raw body in report.failure), request timeout, and a typed OpenApiGetTokenConfig replacing the free-form dict.
2. feat(ingest/openapi): harden malformed-input handling #19906 — malformed-input hardening (base: feat(ingest/openapi): typed get_token config + credential-leak hardening #19905). forced_examples typing/coercion, schema_resolution_max_depth bound, get_swag_json YAML/non-UTF-8/non-object guards, request timeouts, and defensive resolve_schema_references guards.

Per your ask, the follow-ups do not relocate the numeric-bound allOf merge engine or the threaded parser→report bridge — those are dropped entirely (json_schema_util reads none of the numeric keywords, so they had no effect on extracted fields). The full pre-carve branch is preserved locally if we ever want to revisit any of it.

All three (this PR + both follow-ups) are green on ruff/mypy and the test_openapi.py suite.

[acrylJonny]

Follow-up stack is now complete — with the third PR, the stack reproduces this branch's original (pre-strip) content exactly, minus the two pieces called out in review:

1. feat(ingest/openapi): typed get_token config + credential-leak hardening #19905 — typed get_token config + credential-leak hardening
2. feat(ingest/openapi): harden malformed-input handling #19906 — malformed-input hardening (forced_examples/schema_resolution_max_depth validation, request timeouts, get_swag_json YAML/non-UTF8/non-dict guards)
3. feat(ingest/openapi): parser & schema-resolution hardening #19907 — parser & schema-resolution hardening ($defs/JSON-pointer $ref resolution, map-schema normalization, allOf property/oneOf/anyOf/map contribution merging, malformed-input guards, empty-shape detection)

Deliberately dropped (per review), and not in any of the three:

• The numeric-bound allOf merge engine — json_schema_util reads none of the numeric keywords, so it had no effect on extracted fields.
• The threaded parser→report warning bridge — parser degradation now logs as before; source-layer report.warning/report.failure is unchanged.

Verified by diffing the combined stack tip against a preserved snapshot of the original full branch: the only source differences are those two removals. Breaking recipe-validation changes are documented in updating-datahub.md on #19905 and #19906.

[max-datahub]

Approving the carved-down core. I re-read the full net diff (decb902) and it's exactly the right scope:

• json_schema_util — _is_map_schema unifies the string-form / list-form / typeless branches: the hybrid properties + dict additionalProperties case now consistently resolves to object (named fields preserved), and a typeless additionalProperties map resolves to map. It's a shared-module change, but you ran the JsonSchemaTranslator consumers (json-schema / kinesis / kafka / confluent) green, and the kafka golden fixtures are Avro-routed (avro_util, not JsonSchemaTranslator), so no drift. 👍
• openapi_parser — $ref resolution under patternProperties/propertyNames plus map-only promotion to additionalProperties (after the allOf merge, so named properties win), and enum intersection across allOf members. Copy-before-write mutation-safety is preserved throughout.
• Seven focused unit tests, one per distinct behavior — no bloat.

Confirmed the numeric-bound allOf merge engine and the threaded parser→report bridge are absent from this PR (and from the whole follow-up stack — I grepped all four). This is a minimal, self-contained delta on master rather than the original rewrite, with the rest cleanly staged in #19905 / #19906 / #19907.

Non-blocking: the diff currently shows a couple of unrelated updating-datahub.md entries (#19549 / #19554) picked up from a master merge — pure base skew, they'll collapse on squash-merge. A rebase would give a cleaner diff but isn't required.

Nice work on the split — the core fix is tight. Reviewing #19907 next.

• Claude (on behalf of Max)

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 5b3089d. Configure here.}

[cursor]

allOf drops patternProperties maps

Medium Severity

merge_allof_schemas folds properties and additionalProperties from each allOf member but never copies patternProperties. Promotion then runs only on the merged result, so a map defined solely inside allOf (including the common allOf: [{$ref: ...}] form) never becomes additionalProperties and still extracts with no columns.

Additional Locations (1)

• metadata-ingestion/src/datahub/ingestion/source/openapi_parser.py#L899-L903

 

^{Reviewed by Cursor Bugbot for commit 5b3089d. Configure here.}