Skip to content

Fix DeclarativeExecutor passing down empty configs #869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adileo wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix DeclarativeExecutor passing down empty configs #869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
327f788
Use provided config in declarative executor
adileo Nov 16, 2025
9c4f84b
Add config parameter to executor factory function
adileo Nov 16, 2025
6f78a91
Add config parameter to Source initialization
adileo Nov 16, 2025
f7cb109
Apply suggestion from @coderabbitai[bot]
aaronsteers Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion airbyte/_executors/declarative.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def __init__(
self,
name: str,
manifest: dict | Path,
config: dict[str, Any] = {},
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Replace mutable default argument with None.

The linter is correctly flagging B006 here. Using {} as a default is particularly problematic in this case because config_dict is mutated on lines 78-81 when components_py is provided. This means the default dict would accumulate injected components across multiple calls, causing unpredictable behavior. Wdyt about switching to None?

-        config: dict[str, Any] = {},
+        config: dict[str, Any] | None = None,

Then update line 70:

-        config_dict: dict[str, Any] = config
+        config_dict: dict[str, Any] = config if config is not None else {}

Or alternatively at line 70:

-        config_dict: dict[str, Any] = config
+        config_dict: dict[str, Any] = config.copy() if config is not None else {}

(Using .copy() would be safer to avoid mutating the caller's dict, though it depends on whether mutation is intended.)

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Actions: Run Linters

[error] 48-48: B006 Do not use mutable data structures for argument defaults. Replace with None; initialize within function.

🤖 Prompt for AI Agents 
In airbyte/_executors/declarative.py around lines 48 to 48, replace the mutable
default dict in the function signature with None (use config: Optional[dict[str,
Any]] = None and import typing if needed), then at the top of the function
(before lines where config_dict is mutated around 78-81) initialize a fresh dict
and avoid mutating the caller: if config is None: config = {} else: config =
config.copy(); this prevents shared-state bugs and preserves caller data while
allowing local mutations when components_py is provided.

components_py: str | Path | None = None,
components_py_checksum: str | None = None,
) -> None:
Expand All @@ -66,7 +67,7 @@ def __init__(
elif isinstance(manifest, dict):
self._manifest_dict = manifest

config_dict: dict[str, Any] = {}
config_dict: dict[str, Any] = config
if components_py:
if isinstance(components_py, Path):
components_py = components_py.read_text()
Expand Down
3 changes: 3 additions & 0 deletions airbyte/_executors/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ def get_connector_executor( # noqa: PLR0912, PLR0913, PLR0914, PLR0915, C901 #
install_root: Path | None = None,
use_python: bool | Path | str | None = None,
no_executor: bool = False,
config: dict[str, Any] = {}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Replace mutable default argument with None.

Using a mutable default argument ({}) is a common Python pitfall - the same dict instance is shared across all calls that don't provide config, which can lead to unintended state sharing if modified. The linter (B006) is correctly flagging this. Wdyt about using None as the default instead?

-    config: dict[str, Any] = {}
+    config: dict[str, Any] | None = None,

Then at the beginning of the function body (after the docstring), add:

    if config is None:
        config = {}
🤖 Prompt for AI Agents 
In airbyte/_executors/util.py around line 172, the function parameter uses a
mutable default config: dict[str, Any] = {}, which causes shared state across
calls; change the default to None and at the top of the function body (after the
docstring) add a guard that sets config = {} when config is None to avoid shared
mutable defaults.

) -> Executor:
"""This factory function creates an executor for a connector.

Expand Down Expand Up @@ -334,6 +335,7 @@ def get_connector_executor( # noqa: PLR0912, PLR0913, PLR0914, PLR0915, C901 #
return DeclarativeExecutor(
name=name,
manifest=source_manifest,
config=config,
components_py=components_py_path,
)

Expand All @@ -349,6 +351,7 @@ def get_connector_executor( # noqa: PLR0912, PLR0913, PLR0914, PLR0915, C901 #
return DeclarativeExecutor(
name=name,
manifest=manifest_dict,
config=config,
components_py=components_py,
components_py_checksum=components_py_checksum,
)
Expand Down
1 change: 1 addition & 0 deletions airbyte/sources/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ def get_source( # noqa: PLR0913 # Too many arguments
install_if_missing=install_if_missing,
install_root=install_root,
no_executor=no_executor,
config=config,
)

return Source(
Expand Down
Loading