Skip to content

fix(low-code): make Spec.generate_spec idempotent and non-mutating - #1103

Open
Daryna Ishchenko (darynaishchenko) wants to merge 1 commit into
mainfrom
daryna/spec-generate-spec-idempotency
Open

fix(low-code): make Spec.generate_spec idempotent and non-mutating#1103
Daryna Ishchenko (darynaishchenko) wants to merge 1 commit into
mainfrom
daryna/spec-generate-spec-idempotency

Conversation

@darynaishchenko

@darynaishchenko Daryna Ishchenko (darynaishchenko) commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Spec.generate_spec() converted the advanced_auth enum fields (auth_flow_type, nested scopes_join_strategy) to their string values by assigning the converted values back onto the typed model. That in-place mutation meant:

  • a second generate_spec() call in the same process raised AttributeError (.value on an already-converted str) — hit by any flow that generates the spec more than once (Connector Builder, tests);
  • any other reader of self.advanced_auth afterwards saw a string where the type system promises an enum.

This change serializes the model to a dict first and normalizes enum values only in that throwaway copy, so repeated calls are idempotent and the typed model is never mutated. Includes a regression test that calls generate_spec() twice and asserts both idempotency and that the model keeps its enum value.

Context

Split out of #1066, where this fix was bundled with the (unrelated) RateLimitedMultipleTokenAuthenticator feature; review feedback there asked for it to be its own PR. The fix is being removed from #1066.

Testing

  • unit_tests/sources/declarative/spec/test_spec.py — 12 passed, including the new test_generate_spec_is_idempotent_and_does_not_mutate_the_model.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved authentication specification generation for enum-based authentication flows and OAuth scope joining.
    • Ensured repeated specification generation produces consistent results without altering the configured authentication settings.
  • Tests

    • Added regression coverage for repeatable specification generation and protection against configuration mutation.

Previously generate_spec() converted advanced_auth enum fields
(auth_flow_type, scopes_join_strategy) to strings by assigning the
converted values back onto the typed model. A second call in the same
process then raised AttributeError ('.value' on a str), and any other
reader of advanced_auth saw a string where an enum is expected.

Now the model is serialized to a dict first and enum values are
normalized only in that throwaway copy, so repeated calls are
idempotent and the typed model is never mutated. Adds a regression
test that calls generate_spec() twice and asserts the model keeps its
enum.

Split out of #1066 per review feedback, where this fix was bundled
with an unrelated feature.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 5, 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@daryna/spec-generate-spec-idempotency#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 daryna/spec-generate-spec-idempotency

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.

Daryna Ishchenko (darynaishchenko) added a commit that referenced this pull request Aug 5, 2026
Reverts the spec.py enum-handling change and its test to the main
version; the fix now lands separately via #1103, as requested in
review.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Specification generation

Layer / File(s) Summary
Non-mutating authentication serialization
airbyte_cdk/sources/declarative/spec/spec.py, unit_tests/sources/declarative/spec/test_spec.py
Spec.generate_spec copies AdvancedAuth data before converting enum values. The regression test verifies identical repeated results and preserves the original authentication flow type.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: bazarnov

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the idempotency and non-mutation fix in Spec.generate_spec().
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch daryna/spec-generate-spec-idempotency

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
unit_tests/sources/declarative/spec/test_spec.py (1)

165-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the nested enum branch too.

airbyte_cdk/sources/declarative/spec/spec.py normalizes scopes_join_strategy at Lines 65-66, but this test only sets and checks auth_flow_type at Lines 170 and 180. A regression in the nested branch would still pass. Could we add an OAuth input with a non-default scopes_join_strategy, then verify repeated output and preservation of the original nested enum, wdyt?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@unit_tests/sources/declarative/spec/test_spec.py` around lines 165 - 180, The
test_generate_spec_is_idempotent_and_does_not_mutate_the_model test should also
exercise the nested OAuth scopes_join_strategy normalization path. Configure
advanced_auth with a non-default scopes_join_strategy, then assert repeated
generate_spec outputs remain equal and the original nested scopes_join_strategy
enum is unchanged.
airbyte_cdk/sources/declarative/spec/spec.py (1)

58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid using the deprecated Pydantic v2 dump API.

The project declares Pydantic ^2.7 and only ignores ExperimentalClassWarning in tests, so this dict() call can emit deprecation noise. Use model_dump(mode="python") here, wdyt?

Proposed change
-            advanced_auth = self.advanced_auth.dict()
+            advanced_auth = self.advanced_auth.model_dump(mode="python")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@airbyte_cdk/sources/declarative/spec/spec.py` around lines 58 - 68, Replace
the deprecated dict() call in the advanced_auth serialization flow with
Pydantic’s model_dump(mode="python"), preserving the existing Enum normalization
and subsequent oauth configuration handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@airbyte_cdk/sources/declarative/spec/spec.py`:
- Around line 58-68: Replace the deprecated dict() call in the advanced_auth
serialization flow with Pydantic’s model_dump(mode="python"), preserving the
existing Enum normalization and subsequent oauth configuration handling.

In `@unit_tests/sources/declarative/spec/test_spec.py`:
- Around line 165-180: The
test_generate_spec_is_idempotent_and_does_not_mutate_the_model test should also
exercise the nested OAuth scopes_join_strategy normalization path. Configure
advanced_auth with a non-default scopes_join_strategy, then assert repeated
generate_spec outputs remain equal and the original nested scopes_join_strategy
enum is unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5efc3719-ad5c-43b6-b52f-96d9bf8ee265

📥 Commits

Reviewing files that changed from the base of the PR and between 013316a and 725c18f.

📒 Files selected for processing (2)
  • airbyte_cdk/sources/declarative/spec/spec.py
  • unit_tests/sources/declarative/spec/test_spec.py

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

PyTest Results (Fast)

4 159 tests  +1   4 147 ✅ +1   7m 43s ⏱️ -16s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 725c18f. ± Comparison against base commit 013316a.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

PyTest Results (Full)

4 162 tests   4 150 ✅  12m 25s ⏱️
    1 suites     12 💤
    1 files        0 ❌

Results for commit 725c18f.

@tolik0 Anatolii Yatsuk (tolik0) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, well-scoped fix — thanks for splitting it out of #1066.

I verified this locally against the PR head:

  • The bug reproduces on main: a second generate_spec() raises AttributeError: 'str' object has no attribute 'value'.
  • On this branch, two calls compare equal, the model keeps its enum, and the nested scopes_join_strategy path also round-trips correctly ('comma' in the output, ScopesJoinStrategy.comma still on the model).
  • pytest unit_tests/sources/declarative/spec/test_spec.py → 12 passed; mypy, ruff check, and ruff format --check all clean.

The approach is sound. Pydantic v1 .dict() preserves Enum members (no use_enum_values on these models) and builds fresh nested dicts, so the isinstance(..., Enum) guards fire and the in-place mutation of oauth_input correctly propagates into the parent dict. Nothing else in the CDK reads advanced_auth.auth_flow_type, so no caller depended on the old mutation side effect. Dropping the two # type: ignore comments is a nice bonus.

Two things worth addressing before merge (inline), plus one optional simplification.

# Serialize to a dict and normalize enum values there so the typed model is
# never mutated and repeated calls produce the same result
advanced_auth = self.advanced_auth.dict()
if isinstance(advanced_auth.get("auth_flow_type"), Enum):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth calling out in the PR description: this quietly changes behavior when advanced_auth is set but auth_flow_type is None (e.g. a manifest with only predicate_key). AuthFlow is a pydantic model so if self.advanced_auth: is always truthy — previously that path hard-failed with AttributeError: 'NoneType' object has no attribute 'value', and now it emits AdvancedAuth(auth_flow_type=None, ...).

Both AuthFlow.auth_flow_type and the protocol AdvancedAuth.auth_flow_type are Optional, so passing None through is arguably the more correct behavior. But it turns a loud failure into a quiet one for a malformed manifest, and no test pins the intended semantics either way. A one-line test (or a note in the description) would settle it.

Comment on lines +63 to +66
oauth_spec = advanced_auth.get("oauth_config_specification") or {}
oauth_input = oauth_spec.get("oauth_connector_input_specification") or {}
if isinstance(oauth_input.get("scopes_join_strategy"), Enum):
oauth_input["scopes_join_strategy"] = oauth_input["scopes_join_strategy"].value

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Optional simplification — consider letting pydantic normalize enums recursively instead of hand-walking the two known fields:

# Serialize through JSON so enum values are normalized at any depth and the
# typed model is never mutated — repeated calls produce the same result
obj["advanced_auth"] = json.loads(self.advanced_auth.json())

I confirmed this yields 'space' for the nested scopes_join_strategy. declarative_component_schema.py is code-generated and already has ~12 Enum classes, so the current form will silently pass an Enum through to ConnectorSpecificationSerializer the next time an enum field is added anywhere under the advanced_auth subtree.

Trade-off: .json() also coerces other non-JSON types (tuples → lists), none of which exist in this model tree, and it's marginally less explicit about what is normalized. Cost is negligible (once per spec call). Entirely your call — the explicit version is defensible; the generic one just won't need editing again.

Tiny nit while you're here: advanced_auth reads as the model when it's actually the serialized copy — advanced_auth_dict would make the obj["advanced_auth"] = advanced_auth line clearer.

Comment on lines +165 to +180
def test_generate_spec_is_idempotent_and_does_not_mutate_the_model() -> None:
spec = component_spec(
connection_specification={"client_id": "my_client_id"},
parameters={},
advanced_auth=component_auth_flow(
auth_flow_type=component_auth_flow_type.oauth2_0,
predicate_key=None,
predicate_value=None,
),
)

first = spec.generate_spec()
second = spec.generate_spec()

assert first == second
assert spec.advanced_auth.auth_flow_type is component_auth_flow_type.oauth2_0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This covers the auth_flow_type branch but not scopes_join_strategy — which is the subtler half of the change, since it depends on the nested-dict mutation propagating back into advanced_auth. test_declarative_oauth_flow exercises that field, but only for a single generate_spec() call, so a regression in the nested path wouldn't be caught by anything here.

Suggest parametrizing this test over both advanced_auth shapes (or reusing the declarative-OAuth fixture from the case above) so both normalization branches get idempotency coverage. I verified the nested path does work today — this is purely about locking it in.

Also: assert ... is component_auth_flow_type.oauth2_0 is the right check here (identity, not equality) — that catches a mutation to the string "oauth2.0" which == would not.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants