Skip to content

perf(declarative): skip runtime metaschema check of the component schema - #1224

Draft
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1791502007-skip-component-schema-check
Draft

devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1791502007-skip-component-schema-check

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

_validate_source() in ConcurrentDeclarativeSource and the legacy ManifestDeclarativeSource called jsonschema.validators.validate(manifest, schema). In jsonschema 4.x that first runs cls.check_schema(schema), which checks the CDK's own shipped declarative_component_schema.yaml against the Draft 7 metaschema again on every source build. It gets the same answer every time, and it runs a second time when _migrate_manifest validates again.

This PR builds the validator directly, keeps validate's error selection, and only checks the schema when validation fails:

validator_class = validator_for(schema)
try:
    error = best_match(validator_class(schema).iter_errors(manifest))
except Exception:
    validator_class.check_schema(schema)  # a broken schema is reported as SchemaError
    raise
if error is not None:
    validator_class.check_schema(schema)
    raise ValidationError("Validation against json schema defined in declarative_component_schema.yaml schema failed") from error

A valid manifest never reaches check_schema, so it gets the full saving. best_match is kept on purpose: Validator.validate() raises the first error, which is often a different one (see the issue).

The metaschema check also runs in CI: test_the_shipped_component_schema_is_a_valid_draft_7_schema.

Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17933:

Close on merge: yes, internally filed, no customer to confirm with.

Part of https://github.com/airbytehq/airbyte-internal-issues/issues/17920. Requested by Patrick Nilan (Patrick Nilan (@pnilan)).

Timing

I timed validation directly. Each manifest is resolved with ManifestReferenceResolver and ManifestComponentTransformer, the best of 3 runs is kept, and the comparison is old validate() vs new best_match(iter_errors()). Setup: jsonschema 4.25.1, the 516 manifest.yaml files under airbytehq/airbyte@79bef47d, and a dev VM.

p50 p90
Before: validate() 68.7 ms 94.6 ms
After: best_match(validator.iter_errors()) 5.8 ms 32.0 ms
Draft7Validator.check_schema(component_schema) alone (min of 5) 64.1 ms

All 516 manifests are valid, so none of them reaches the failure-path check_schema. The outcome was identical for all 516. Absolute numbers on this VM are higher than the issue's 1-CPU container numbers (25 ms saving). The saved step is the same in both measurements. An invalid manifest still pays for check_schema once, as it did before.

Test Coverage

  • test_the_shipped_component_schema_is_a_valid_draft_7_schema (test_declarative_component_schema_is_loadable.py): asserts that validator_for(schema) is Draft7Validator, then runs check_schema.
  • test_invalid_manifest_raises_the_best_match_error_not_the_first (concurrent and legacy): with the shipped schema, the manifest has two errors. The first error from iter_errors is at $.streams[0] (a guard assertion checks this), and the best match is $.streams[1].retriever.requester.http_method (enum). The test checks that the wrapper message is unchanged. It also checks that __cause__ matches what jsonschema.validate raises on the same manifest in message, json_path, schema_path, validator and validator_value.
  • test_building_a_source_does_not_check_the_component_schema (concurrent and legacy): patches Draft7Validator.check_schema to raise, then builds a valid source.
  • test_a_broken_component_schema_that_fails_the_manifest_raises_schema_error (concurrent and legacy): HttpRequester.http_method gets enum: "GET", so a valid manifest fails validation. Building the source raises SchemaError.
  • test_a_broken_component_schema_that_breaks_validation_raises_schema_error (concurrent and legacy): HttpRequester.path gets type: strng, so iter_errors raises UnknownType (a guard checks this). Building the source raises SchemaError.

Verified against unfixed code: both does_not_check tests fail on main, because validate() calls check_schema. The four broken-schema tests fail on this branch's first commit, which raised ValidationError and UnknownType instead. The best-match tests and the metaschema test pass on main too, which shows the error is the same as before. With the fix, all pass: 139 passed, 1 skipped across the three test files. On the first commit I also ran unit_tests/sources/declarative, unit_tests/legacy and unit_tests/connector_builder (2547 passed, 1 skipped). Ruff and mypy pass on the changed source files.

Risk

  • Broken schema: a malformed component schema is still reported as SchemaError, because check_schema runs before any validation error or exception is raised. Only a CDK change can break the schema, since it ships inside the CDK wheel. The metaschema test catches that before release.
  • Malformed schema that accepts the manifest: if a broken schema still lets the manifest validate, no error is raised at runtime. The metaschema test catches that case in CI.
  • Out of scope: ValidateAdheresToSchema and schema_helpers are intentionally unchanged, because their schemas come from the manifest or the connector.

Link to Devin session: https://app.devin.ai/sessions/22162a3a9629407c95ac5ff107c3f0af
Open in Devin Desktop: https://app.devin.ai/desktop/session/22162a3a9629407c95ac5ff107c3f0af?variant=devin

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

Testing This CDK Version

You can test this version of the CDK using the following:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1791502007-skip-component-schema-check#egg=airbyte-python-cdk[dev]' --help

# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1791502007-skip-component-schema-check

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment
📚 Show Repo Guidance

Helpful Resources

📝 Edit this welcome message.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

PyTest Results (Fast)

5 072 tests  +38   5 060 ✅ +38   10m 2s ⏱️ -39s
    1 suites ± 0      12 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 29b03be. ± Comparison against base commit 916d6f6.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

PyTest Results (Full)

5 075 tests  +38   5 063 ✅ +38   15m 43s ⏱️ +11s
    1 suites ± 0      12 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 29b03be. ± Comparison against base commit 916d6f6.

♻️ This comment has been updated with latest results.

@pnilan

Copy link
Copy Markdown
Collaborator

🤖 This comment was generated by an AI Agent.

The issue this PR resolves has one addition, under "Proposed change", "Risks to cover" and "Acceptance criteria" of airbytehq/airbyte-internal-issues#17933. Please update the PR for it.

Check the schema on the failure path

When validation finds an error, or raises, call check_schema before reporting, in both _validate_source implementations:

validator_class = validator_for(schema)
try:
    error = best_match(validator_class(schema).iter_errors(manifest))
except Exception:
    validator_class.check_schema(schema)  # a broken schema is reported as SchemaError
    raise
if error is not None:
    validator_class.check_schema(schema)
    raise ValidationError(...) from error

A valid manifest never reaches check_schema, so the saving is unchanged and test_building_a_source_does_not_check_the_component_schema still holds.

Why: a CDK developer who breaks declarative_component_schema.yaml locally gets an error that blames the manifest. On main each of these raises SchemaError; on this branch, with a valid manifest:

Mistake in the schema Error on this branch
enum: GET where a list belongs, on HttpRequester.http_method ValidationError: 'GET' is not one of 'GET'
type: strng UnknownType
properties: [] AttributeError: 'list' object has no attribute 'items'

Please cover, for the concurrent and the legacy source:

  • a schema that violates its metaschema and makes a manifest fail validation raises SchemaError;
  • a schema that violates its metaschema and makes validation raise (type: strng) raises SchemaError;
  • with the shipped schema, an invalid manifest raises the same error as it does now.

The "Risk" section of the description will need updating too.

…n fails

Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Patrick Nilan (@pnilan) Done in 29b03be, reply to this comment.

  • Both _validate_source implementations now call validator_class.check_schema(schema) before raising. This happens when best_match finds an error, and when iter_errors raises (then the original exception is re-raised). A valid manifest never reaches check_schema, and test_building_a_source_does_not_check_the_component_schema still passes.
  • New tests, for both the concurrent and the legacy source:
    • test_a_broken_component_schema_that_fails_the_manifest_raises_schema_error: HttpRequester.http_method gets enum: "GET". A guard asserts that the valid manifest now has errors. Building the source raises SchemaError.
    • test_a_broken_component_schema_that_breaks_validation_raises_schema_error: HttpRequester.path gets type: strng. A guard asserts that iter_errors raises UnknownType. Building the source raises SchemaError.
    • The shipped-schema case is the existing test_invalid_manifest_raises_the_best_match_error_not_the_first, unchanged. It compares the cause against what jsonschema.validate raises.
  • All four new tests failed on the previous commit (ValidationError / UnknownType), and all pass now: 139 passed, 1 skipped across the three test files. Ruff and mypy are clean.

I updated the Summary, Test Coverage and Risk sections of the description to match.

Written by Devin

@pnilan Patrick Nilan (pnilan) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by an AI Agent.

Review: fix before merge

The change is correct and non-breaking. For a valid manifest, _validate_source is now jsonschema.validate() without check_schema, and results matched in 56,238 cases across all 21 jsonschema releases that >=4.17.3,<5.0 allows. The new tests also catch the regression they target: on main only the two does_not_check tests fail, and on ff64dbe only the four broken-schema tests fail.

Please address the items below. Items 1–3 are needed before merge. Items 4–6 are smaller fixes to make in the same pass.

1. Keep a traceback on the chained cause (P2)

concurrent_declarative_source.py:360-364 and manifest_declarative_source.py:462-466

The failure-path structure requested earlier in this thread has a side effect. best_match() returns an error that is never raised, so __cause__.__traceback__ is None. traceback.format_exc() then prints the cause without a Traceback (most recent call last): header, and the whole string starts with jsonschema.exceptions.ValidationError: ….

The SDM sends that string as the TRACE stack_trace (_run.py:235, and :121 for bundled manifests). The platform's Sentry reporter parses Python frames only when the trace starts with that header (SentryExceptionHelper.kt:66). Otherwise it tags stacktrace_parse_error=1 and groups by message text (SentryJobErrorReportingClient.kt:92), so invalid-manifest failures stop grouping by stack frame. The exception type, wrapper message and failure_type are unchanged, so job outcomes are not affected.

Raise the best match inside the try instead, in both files:

validator_class = validator_for(self._declarative_component_schema)
try:
    error = best_match(
        validator_class(self._declarative_component_schema).iter_errors(self._source_config)
    )
    if error is not None:
        raise error
except ValidationError as e:
    validator_class.check_schema(self._declarative_component_schema)
    raise ValidationError(
        "Validation against json schema defined in declarative_component_schema.yaml schema failed"
    ) from e
except Exception:
    validator_class.check_schema(self._declarative_component_schema)
    raise

I tested this. The trace starts with the header again (4 frames, against 5 on main; only jsonschema's internal frame is gone), and all 140 tests in the three changed test files pass. Also add assert cause.__traceback__ is not None to both test_invalid_manifest_raises_the_best_match_error_not_the_first tests. They compare every field of the cause except its traceback, which is how this got through.

2. Retitle the PR (P2)

perf is not an allowed type in semantic_pr_check.yml. The required Validate PR title check skipped both of its steps because the PR is a draft, so its green status means nothing yet. It will fail once the PR is marked ready for review. Use:

fix(declarative): speed up manifest validation at source startup

The released version is the same either way: the release drafter bumps patch for both perf and fix.

3. Merge main and re-run CI

One line conflicts in unit_tests/sources/declarative/test_concurrent_declarative_source.py: #1145 added TestLimits, to the same import block. Keep both names:

from airbyte_cdk.sources.declarative.concurrent_declarative_source import (
    ConcurrentDeclarativeSource,
    TestLimits,
    _get_declarative_component_schema,
)

The current CI results are for a base 10 commits behind main (916d6f6).

4. Tighten the new tests (P2)

  • Re-raise branch. No test covers a schema that passes check_schema but makes iter_errors raise (:357-359, legacy :459-461). With raise deleted, all 8 source tests still pass, and the code would raise UnboundLocalError. Add a broken-schema case with a dangling $ref: HttpRequester.properties.path = {"$ref": "#/definitions/DoesNotExist"}. Assert that check_schema passes on it, then that building the source raises the same exception type as list(Draft7Validator(broken).iter_errors(manifest)).
  • Version-dependent assertions. cause.json_path == "$.streams[1].retriever.requester.http_method" (concurrent test :7032, legacy :2606) and the enum: "GET" premise (:7054, :2623) fail on jsonschema 4.17.3, the pyproject.toml floor; 4.17.3–4.20.0 check enum as a substring. They pass in CI only because of the 4.25.1 lock. Keep the first error from the guard and assert cause.json_path != first.json_path, and break the schema with required: "type" instead of enum: "GET".
  • Validator class. patch.object(Draft7Validator, "check_schema", ...) (:7037, :2611) stops guarding anything if the schema's $schema changes. Patch validator_for(schema) instead, and add assert_not_called() after building the source.
  • Migration path. Builder and the manifest server build sources with migrate_manifest=True, which validates twice. Parametrize both does_not_check tests over migrate_manifest.

5. Remove the duplication (P3)

The new _validate_source body, its fixture and its four tests are copied into both modules, so every change in item 4 has to be made twice. Add a module-level _validate_manifest(manifest, schema) next to _get_declarative_component_schema in concurrent_declarative_source.py, and call it from both classes. Pass schema as a parameter so the existing patches of each module's _get_declarative_component_schema keep working. The legacy tests can then shrink to one test that checks the call is delegated, and the item 4 changes only need to be made once.

6. Fix the wording (P3)

  • The comment at :349 (legacy :451) says "On failure the schema is checked first", which describes the old order. Suggested: "If validation fails or raises, check_schema runs before the error propagates, so a broken schema surfaces as SchemaError instead of being blamed on the manifest."
  • The docstring of test_the_shipped_component_schema_is_a_valid_draft_7_schema says _validate_source skips check_schema at runtime, but it still runs whenever validation fails. Suggested: "_validate_source runs check_schema only when a manifest fails validation, so a malformed schema that still accepts manifests is caught only here."
  • The module docstring's "Two guards" (test_declarative_component_schema_is_loadable.py:15) no longer counts the new test.

Then update the Summary snippet and the Risk section of the description to match.

Check: destination-motherduck fails on a MotherDuck token authentication error, the same as on #1221, #1223 and #1145. It is unrelated to this PR and needs no action.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant