Repository navigation
fix(cloud): Unwrap {"manifest": ...} envelope in builder draft_manifest - #1381
Conversation
get_with_manifest returns the stored draft under
declarativeManifest.manifest; drafts persisted as a
DeclarativeSourceManifest envelope surface as {"manifest": {...}, ...}
while the published manifest is bare. draft_manifest now unwraps that
envelope so both shapes match.
Co-Authored-By: AJ Steers <[email protected]>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1792014276-builder-draft-manifest-shape' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1792014276-builder-draft-manifest-shape'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
There was a problem hiding this comment.
🟢 Approval recommended
The narrowly scoped normalization matches the intended behavior, with no unresolved findings.
0 open findings
What changed in this PR
Normalizes connector builder draft manifests so wrapped drafts have the same shape as published manifests.
Changes:
- Adds a shared unwrapping helper for draft reads and renames.
- Preserves bare manifests and missing-draft handling.
- Adds regression coverage for wrapped and bare drafts.
| File | Description |
|---|---|
| tests/unit_tests/test_cloud_credentials.py | Tests wrapped and bare draft manifest reads. |
| airbyte/cloud/connectors.py | Normalizes builder manifests in draft_manifest and rename(). |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code coverage overviewLanguages: Python Python / code-coverage/pytest-no-creds
View coverage of the 10 most impacted files.
Python / code-coverage/pytest
View coverage of the 10 most impacted files.
Python / code-coverage/pytest-unit
Line coverage data for the release-candidate/v1 branch is not yet available. View coverage of the 10 most covered files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
cc28ed4
into
release-candidate/v1
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| builder_project = project["builderProject"] | ||
| draft_manifest = ( | ||
| project["declarativeManifest"]["manifest"] | ||
| _unwrap_builder_manifest(project["declarativeManifest"]["manifest"]) |
There was a problem hiding this comment.
🔴 Rename drops saved draft metadata
When a draft uses an envelope, rename() sends only its nested manifest to update_connector_builder_project. The replacement draftManifest loses saved envelope fields such as spec and version.
Learn more
A builder draft can store a DeclarativeSourceManifest object with both a nested manifest and separate envelope fields. The rename operation reads that saved draft and sends draft_manifest to update_connector_builder_project, which writes it back as builderProject.draftManifest. Unwrapping the draft before this write drops the other envelope fields from storage. The read-only draft_manifest property can still return a bare manifest without changing the stored draft.
Example: A saved draft of {"manifest": {"type": "DeclarativeSource", "version": "0.1.0"}, "spec": {"connectionSpecification": {}}, "version": 1} becomes {"type": "DeclarativeSource", "version": "0.1.0"} after a rename; its separate spec is lost.
Recommended fix: Pass the original project["declarativeManifest"]["manifest"] into the update operation during rename. Unwrap only when returning draft_manifest to callers, and add a rename test that asserts the full envelope reaches the update request.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed: this is real. rename() sends the unwrapped manifest back as draftManifest. For a draft stored in the {"manifest": ..., "spec": ..., "version": ...} envelope, the rename therefore drops the envelope fields, including any spec that exists only on the envelope.
Rename shouldn't rewrite the stored draft. The minimal fix is to send the raw project["declarativeManifest"]["manifest"] during rename and unwrap only in the read-only draft_manifest property. That needs a rename test asserting the full envelope reaches update_connector_builder_project. This PR is already merged, so I've asked AJ whether to do it as a follow-up PR.
Overview
👉 TL;DR: The draft manifest of a connector builder project could come back wrapped in an extra layer, while the published manifest came back bare. Both now have the same shape.
Specifically, a new
_unwrap_builder_manifest()is used byCustomCloudSourceDefinition.draft_manifestand byrename().Changes
get_with_manifestreturns the storedmanifestDraftJSON unchanged indeclarativeManifest.manifest(airbyte-platform-internalConnectorBuilderProjectsHandler.kt:318). Drafts saved as aDeclarativeSourceManifestenvelope ({"manifest": {...}, "spec": ..., ...}) came out wrapped.manifestis a dict and none of the top-level keys of a bare manifest (streams,definitions,check) are present. Bare manifests andNonepass through unchanged.Test plan
Link to Devin session: https://app.devin.ai/sessions/a51d808f26c9402fafc93e7640648187
Open in Devin Desktop: https://app.devin.ai/desktop/session/a51d808f26c9402fafc93e7640648187?variant=devin
Requested by: Aaron ("AJ") Steers (@aaronsteers)