fix(declarative): stop coercing static scalar strings in request_body_json - #1081
Draft
Airbyte Support (Airbyte-Support) wants to merge 2 commits into
Draft
fix(declarative): stop coercing static scalar strings in request_body_json#1081Airbyte Support (Airbyte-Support) wants to merge 2 commits into
Airbyte Support (Airbyte-Support) wants to merge 2 commits into
Conversation
…d of coercing them Co-Authored-By: syed.khadeer@airbyte.io <cloud-support@airbyte.io>
Contributor
🤖 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/1785206396-nested-mapping-static-strings#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/1785206396-nested-mapping-static-stringsPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)4 144 tests +11 4 132 ✅ +11 7m 53s ⏱️ +18s Results for commit ac74d0a. ± Comparison against base commit 7e96546. This pull request removes 13 and adds 24 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)4 147 tests +11 4 135 ✅ +11 12m 35s ⏱️ +12s Results for commit ac74d0a. ± Comparison against base commit 7e96546. This pull request removes 13 and adds 24 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Co-Authored-By: syed.khadeer@airbyte.io <cloud-support@airbyte.io>
devin-ai-integration
Bot
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A Connector Builder user reported that
RequestBodyJsonObjectwithclientId: "478"sends{"clientId": 478}— a number — and that no amount of YAML quoting or Jinja ("{{ '478' }}") fixes it, breaking an API that requires a string.Cause: every string in
request_body_jsonis routed throughJinjaInterpolation.eval, which runsast.literal_evalon the rendered output. For a static YAML literal there is nothing to render, so the only effect of that round-trip is type coercion. The result looks arbitrary to users: in the reporter's own manifest,storeId: ["478-1", "478-2", ...]survives (not valid Python literals) whileclientId: "478"becomes478, and"None"becomesNoneand is then dropped from the body entirely.The naive fix — skip
literal_evalfor all static strings — is too broad, and CI caught why:source-intercomdeliberately encodes structure as a string and depends on it being parsed.So the rule is narrowed to structured vs. scalar: a static string keeps its literal-eval result only when that result is a
dictorlist; scalars are returned verbatim. Encoding a JSON object/array as a string is a deliberate, meaningful use ofliteral_eval; scalar coercion never is."478"478"478""0012""0012""0012""None""None"'{"field": "updated_at"}'{"field": ...}{"field": ...}"[1, 2, 3]"[1, 2, 3][1, 2, 3]"{{ next_page_token['offset'] }}"intintThe change is confined to
InterpolatedNestedMapping, which is only wired intoInterpolatedNestedRequestInputProvider(request body JSON).jinja.pyandInterpolatedStringare deliberately untouched — changing_literal_evalglobally would affect request params, headers, paths, and every other interpolation caller.Rollout risk: a static quoted scalar number in
request_body_jsonnow stays a string. A scan of every declarative manifest inairbytehq/airbytefound exactly one affected value:source-lookermanifest.yaml:26,request_body_json.limit: "10000", which becomes"10000". No quoted"True"/"False"/"None"values exist anywhere.Test plan
test_interpolated_nested_mapping.py: scalars preserved (as dict keys, nested in dicts, inside lists), structured JSON object/array strings still parsed, a regression case mirroring intercom'ssort, and"{{ 1 + 1 }}"still evaluating to2.test_interpolated_request_options_provider.pyassertingRequestBodyJsonObject(value={"clientId": "478"})yields{"clientId": "478"}.ruff,mypy,pytest unit_tests/sources/declarative, and all 27 CI checks pass — includingsource-intercom, which fails on the un-narrowed version.Reported by Syed Khadeer.
Link to Devin session: https://app.devin.ai/sessions/ba9cc31475fb453fba21067b02845c73
Requested by: Airbyte Support (@Airbyte-Support)