Skip to content

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

Merged
Patrick Nilan (pnilan) merged 15 commits into
devin/1791499901-yaml-csafeloaderfrom
devin/1791502007-skip-component-schema-check
Oct 22, 2026
Merged

Patrick Nilan (pnilan) merged 15 commits into
devin/1791499901-yaml-csafeloaderfrom
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 adds a shared validate_manifest(manifest, schema) in concurrent_declarative_source.py, called by both _validate_source implementations. It builds the validator directly, keeps validate's error selection, and only checks the schema when validation fails or raises:

validator_class = validator_for(schema)
try:
    error = best_match(validator_class(schema).iter_errors(manifest))
    if error is not None:
        raise error  # raised, so the chained cause keeps a traceback
except ValidationError as e:
    validator_class.check_schema(schema)  # a broken schema surfaces as SchemaError
    raise ValidationError("Validation against json schema defined in declarative_component_schema.yaml schema failed") from e
except Exception:
    validator_class.check_schema(schema)
    raise

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

The validation tests live in test_concurrent_declarative_source.py. The legacy source delegates to the same function.

  • 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: with the shipped schema, the manifest has two errors.
    • The best match must differ from the first error that iter_errors returns.
    • The cause must match what jsonschema.validate raises in message, json_path, schema_path, validator and validator_value.
    • The cause must have a traceback.
    • The wrapper message is unchanged.
  • test_building_a_source_does_not_check_the_component_schema (concurrent and legacy, parametrized over migrate_manifest): patches check_schema on validator_for(schema) and asserts it is not called.
  • test_a_broken_component_schema_that_fails_the_manifest_raises_schema_error: HttpRequester.required: "type" makes a valid manifest fail, and building the source raises SchemaError.
  • test_a_broken_component_schema_that_breaks_validation_raises_schema_error: type: strng makes iter_errors raise UnknownType, and building the source raises SchemaError.
  • test_a_component_schema_that_passes_check_schema_but_breaks_validation_reraises: a dangling $ref passes check_schema but makes iter_errors raise. The same exception type is re-raised. With the bare raise removed, this test fails.
  • test_legacy_source_delegates_to_public_manifest_validator (legacy): checks that the legacy source calls the shared function.

Results:

  • Against unfixed code: both does_not_check tests fail on main, and the broken-schema tests failed on this branch's first commit.
  • With the fix, the three test files pass: 143 passed, 1 skipped on jsonschema 4.25.1.
  • On jsonschema 4.17.3 (the pyproject.toml floor), the new tests pass. The only failure is the existing test_combined_extractor_with_an_empty_extractors_list_fails_schema_validation from feat: add CombinedExtractor for combining multiple record extractors #1162, whose expected message wording differs on that version.
  • Ruff and mypy pass on the changed source files.

Risk

  • Broken schema: a malformed component schema is still reported as SchemaError whenever a manifest fails validation or validation raises. Only a CDK change can break the schema, since it ships inside the CDK wheel.
  • Malformed schema that accepts the manifest: if a broken schema still accepts the manifest, nothing is raised at runtime. test_the_shipped_component_schema_is_a_valid_draft_7_schema catches that case in CI.
  • Traceback: the best match is raised inside validate_manifest, so the chained cause keeps a traceback and the TRACE stack_trace still starts with Traceback (most recent call last):.
  • 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 Bot and others added 3 commits October 8, 2026 22:18
…nifest image

Co-Authored-By: bot_apk <apk@cognition.ai>
…eLoader when available

Co-Authored-By: bot_apk <apk@cognition.ai>
@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 864 tests  +10   5 833 ✅ +10   5m 34s ⏱️ -10s
    1 suites ± 0      31 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 20d530b. ± Comparison against base commit 766f1d3.

♻️ 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 867 tests  +10   5 836 ✅ +10   10m 19s ⏱️ -44s
    1 suites ± 0      31 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 20d530b. ± Comparison against base commit 766f1d3.

♻️ This comment has been updated with latest results.

@pnilan

Copy link
Copy Markdown
Member

🤖 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.

devin-ai-integration Bot and others added 2 commits October 9, 2026 17:13
…r fails

Co-Authored-By: bot_apk <apk@cognition.ai>
…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
Member

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.

devin-ai-integration Bot and others added 3 commits October 21, 2026 19:30
…-component-schema-check

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

# Conflicts:
#	unit_tests/sources/declarative/test_concurrent_declarative_source.py
Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration devin-ai-integration Bot changed the title perf(declarative): skip runtime metaschema check of the component schema fix(declarative): speed up manifest validation at source startup Oct 21, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Patrick Nilan (@pnilan) I addressed your review in c9fb2fb (merge of main) and 54d193a.

  1. Traceback: the best match is now raised inside the try, using your structure. traceback.format_exc() for an invalid manifest starts with Traceback (most recent call last): again, and the best-match test asserts cause.__traceback__ is not None.
  2. Title: changed to fix(declarative): speed up manifest validation at source startup.
  3. Merge: merged main and resolved the import conflict by keeping TestLimits alongside _get_declarative_component_schema.
  4. Tests:
    • Added a dangling-$ref case (test_a_component_schema_that_passes_check_schema_but_breaks_validation_reraises). It fails with DID NOT RAISE _WrappedReferencingError when the bare raise is removed.
    • The best-match test now asserts cause.json_path != first.json_path instead of a hard-coded path.
    • The broken-schema case now uses HttpRequester.required: "type".
    • The does_not_check tests patch validator_for(schema), call assert_not_called(), and are parametrized over migrate_manifest.
    • On jsonschema 4.17.3, the new tests pass. The one failure there is the existing test_combined_extractor_with_an_empty_extractors_list_fails_schema_validation from feat: add CombinedExtractor for combining multiple record extractors #1162, which expects 4.25 wording (should be non-empty vs [] is too short).
  5. Duplication: _validate_manifest(manifest, schema) now lives next to _get_declarative_component_schema, and both sources call it. The legacy tests are down to the parametrized does_not_check test plus test_validate_source_delegates_to_validate_manifest.
  6. Wording: I updated the code comment, the metaschema test's docstring and the "Three guards" module docstring as you suggested, plus the Summary, Test Coverage and Risk sections of the description.

Results: the three changed test files pass (143 passed, 1 skipped on 4.25.1), and Ruff and mypy pass. CI is re-running on the new base.

Written by Devin

Comment thread airbyte_cdk/sources/declarative/concurrent_declarative_source.py Outdated
Comment thread unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py Outdated
@pnilan
Patrick Nilan (pnilan) marked this pull request as ready for review October 21, 2026 20:29
@pnilan
Patrick Nilan (pnilan) requested a review from a team as a code owner October 21, 2026 20:29
devin-ai-integration Bot and others added 2 commits October 21, 2026 20:40
Co-Authored-By: bot_apk <apk@cognition.ai>
…est dirs

Co-Authored-By: bot_apk <apk@cognition.ai>
@pnilan
Patrick Nilan (pnilan) changed the base branch from main to devin/1791499901-yaml-csafeloader October 21, 2026 21:58
devin-ai-integration Bot and others added 3 commits October 21, 2026 21:59
…precompile-bytecode

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

# Conflicts:
#	Dockerfile
…91499901-yaml-csafeloader

Co-Authored-By: bot_apk <apk@cognition.ai>
…-component-schema-check

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

Copy link
Copy Markdown
Member

/ai-resolve-conflicts

…7-skip-component-schema-check

Update test_yaml_loader.py to the public get_declarative_component_schema.

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

Copy link
Copy Markdown
Contributor Author

I merged the rebased stack base devin/1791499901-yaml-csafeloader into this branch in 4e78a0e. There were no textual conflicts.

There was one break that didn't show up as a conflict. The base adds unit_tests/utils/test_yaml_loader.py, which imports _get_declarative_component_schema from concurrent_declarative_source. This PR renamed that function, so pytest collection failed with an ImportError. The test now imports the public get_declarative_component_schema.

I left its import of the legacy module's private loader unchanged, since this PR doesn't rename that one. test_yaml_loader.py and the related declarative tests pass locally.

Written by Devin

@pnilan

Patrick Nilan (pnilan) commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/39684746365

…r' into devin/1791502007-skip-component-schema-check

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

# Conflicts:
#	unit_tests/utils/test_yaml_loader.py
@pnilan
Patrick Nilan (pnilan) enabled stacked auto-merge (squash) October 22, 2026 02:31
@pnilan
Patrick Nilan (pnilan) merged commit c60b837 into main Oct 22, 2026
26 of 28 checks passed
@pnilan
Patrick Nilan (pnilan) deleted the devin/1791502007-skip-component-schema-check branch October 22, 2026 02:39
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