Repository navigation
fix: treat None-valued conditions as false in InterpolatedBoolean - #1152
Conversation
Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou 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/1789406745-interpolated-boolean-none#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/1789406745-interpolated-boolean-nonePR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate correctness issue remains because string values rendered as None may be incorrectly treated as false.
Pull request overview
Updates declarative boolean interpolation so None conditions evaluate as false, with regression coverage.
Changes:
- Adds
NonetoInterpolatedBoolean.FALSE_VALUES. - Tests missing, explicit-null, short-circuit, and truthy fallback cases.
File summaries
| File | Summary |
|---|---|
unit_tests/sources/declarative/interpolation/test_interpolated_boolean.py |
Adds regression tests for null and short-circuit conditions. |
airbyte_cdk/sources/declarative/interpolation/interpolated_boolean.py |
Treats interpolated None values as false. |
Review details
Suppressed comments (1)
airbyte_cdk/sources/declarative/interpolation/interpolated_boolean.py:22
- Adding the Python
Nonesentinel here also classifies any rendered string exactly equal toNoneas false:JinjaInterpolation._literal_evalparses the output of{{ 'None' }}(and a config value'None') intoNonebefore this membership check. That expression/value is truthy in both Jinja and Python, so this introduces a false negative beyond the intended null case. Preserve the original Jinja value/type, or otherwise distinguish a real null from a string rendering, and add a regression test.
None,
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
👉 TL;DR: Declarative
conditionexpressions that evaluate to "nothing" (a missing config key, an explicitnull, or anand/orchain that short-circuits to nothing) were being treated as true. They now evaluate to false, matching how Jinja's own{% if %}and Python truthiness behave.Specifically,
Noneis added toInterpolatedBoolean.FALSE_VALUESsoInterpolatedBoolean.evalreturnsFalsewhenJinjaInterpolation.evalyieldsNone.Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17241:
Related to https://github.com/airbytehq/airbyte-internal-issues/issues/17212 (source-linear OAuth regression caused by this behavior; connector-side workaround in airbytehq/airbyte#85789).
Changes
airbyte_cdk/sources/declarative/interpolation/interpolated_boolean.py: addNonetoFALSE_VALUES.unit_tests/sources/declarative/interpolation/test_interpolated_boolean.py: add cases for{{ none }},.get()on a missing key, a key present withnull,and/orchains that short-circuit toNone(allFalse), and anorchain that falls through to a truthy value (True). The fiveFalsecases fail onmainand pass with this change.Why
JinjaInterpolation.evalrenders the template to a string andast.literal_evals it, so{{ config.get('api_key') }}renders"None"and comes back as PythonNone.Nonewas not inFALSE_VALUESandNone == FalseisFalse, soevalfell through toreturn True. Reproduced onmain@ 95439d6:This bit source-linear: a
ConfigAddFieldscondition{{ config.get('api_key') }}fired for OAuth configs and wrote an emptycredentials.api_key, breaking OAuth token persistence. The connector-side workaround (config.get('api_key', False)) still misfires whenapi_keyis explicitlynull; this change closes that gap at the CDK level.Behavior change / blast radius
This is a behavior change for any manifest condition that currently relies on a
None-valued expression being truthy. From the issue author's scan of 534 manifests inairbytehq/airbyte:basesstreamRecordFilter{{ record.get("permissionLevel") }}— bases withoutpermissionLevelwould now be filtered out.stop_condition: {{ response.get("end_of_stream") }}— pagination continues (rather than stops) whenend_of_streamis absent but a next cursor exists.null.Monorepo connectors pin their CDK version, so those two should be reviewed when they next bump the CDK. Custom Builder connectors receive minor/patch CDK releases automatically via
DeclarativeSourceUpdater, so this note is the user-facing changelog signal.Versioning decision
I've titled this
fix:(patch) rather thanfix!:(major). Reasoning: the change alignsInterpolatedBooleanwith the documented Jinja/Python semantics that manifest authors already expect, the previous behavior was a bug that produced incorrect connector behavior (17212), and no public interface changes. If maintainers would rather ship this as a major release given the Builder auto-update path, retitling tofix!:is the only change needed.fix:(patch) vsfix!:(major) for this behavior change.Test plan
poetry run pytest unit_tests/sources/declarative/interpolation/ -q— 196 passed.poetry run pytest unit_tests/sources/declarative/ -q— full declarative suite (in progress locally; will follow up with results on this PR).poetry run ruff check .andpoetry run ruff format --check .— clean.Requested via the
/ai-fixplaybook from the triage session for issue 17241 (issue author: Patrick Nilan (@pnilan)).Link to Devin session: https://app.devin.ai/sessions/5369826e03544650b0c3edd431cef88e
Open in Devin Desktop: https://app.devin.ai/desktop/session/5369826e03544650b0c3edd431cef88e?variant=devin