fix(destination-postgres): strip NUL characters nested in JSON values - #84321
fix(destination-postgres): strip NUL characters nested in JSON values#84321devin-ai-integration[bot] wants to merge 3 commits into
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:
|
Co-Authored-By: bot_apk <apk@cognition.ai>
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
|
|
Deploy preview for airbyte-docs ready!
Deployed with vercel-action |
|
↪️ Triggering Reason: Draft fix with all CI checks green; prove-fix validation is the next pipeline step for: |
|
🟢 Fix ProvenPre-release tested: An isolated A/B reproduction confirms the reported failure on Root cause confirmed
EvidenceLocal Postgres 15.18 in Docker, driven at the Airbyte protocol level. Byte-identical input to both images, differing only in target schema name. Three records: one with a NUL nested four levels deep in a Path 1 —
Path 2 —
Data integrity on the target
Regression checkPyAirbyte destination smoke test against the pre-release: Pre-flight checks
What was not testedNo live customer sync was run. Approval to pin the pre-release on the reporting connection was requested but has not come back, so nothing was pinned. The isolated A/B stands on its own — it reproduces the exact failure on the baseline and shows it resolved on the pre-release — but it uses a synthetic record shaped like the reported payload rather than the original data. One detail worth flagging for review: the recursive sanitizer returns the original instance when no descendant changed, so unaffected records avoid a full tree copy. That keeps the hot path allocation-free for the common case, but it does mean the returned value is sometimes the same object and sometimes a copy — fine given RecommendationMerge and roll out. Progressive rollout is already enabled. The reporting connection should recover on its next sync with no manual intervention — no state reset or backfill needed, since the failing Customer-specific details are recorded privately in the linked oncall issue. Session: https://app.devin.ai/sessions/472e51ec087646e090f7b230d0aacf9e |
Airbyte Support Bot (airbyte-support-bot)
added
the
hyd-prove
|
|
↪️ Triggering Reason: |
Reviewing PR for connector safety and quality.
|
🛡️ AI PR Review Report🟢 Review Action: APPROVED
All other gates passed. 🔶 Risk Level: 3/5Logic change in the per-record write path of an existing connector ( 📋 PR DetailsConnector(s): 🔍 Gate Evaluation DetailsGate-by-Gate Analysis
Notes on evidence scoping
📚 Evidence ConsultedEvidence
|
|
🚫 Not fixing the per-record performance warning about The extra allocation is real but marginal in context: the immediately preceding On the other flagged detail: |
|
↪️ Triggering Reason: AI review returned APPROVE for the exact current HEAD |
Auto-merge evaluation: FAILEligibility (all must pass)
|
|
🙋 Escalated to #human-in-the-loop per Hands-Free AI Triage Project triage next step. Reason: Every automated gate passed — |
| if ('\u0000' in value.value) { | ||
| SanitizedValue(StringValue(value.value.replace("\u0000", "")), true) |
There was a problem hiding this comment.
why the if statement? is in cheaper than a no-op replace?
There was a problem hiding this comment.
🙋 Your call — happy to simplify. The if isn't there for the cost of replace; it's there because I need the changed boolean, which is what lets parent ObjectValue/ArrayValue nodes skip copying their container when no descendant was modified. So the check does double duty as "did anything change here".
On the cost question specifically: you're right that the scan isn't buying much. '\u0000' in s is indexOf(Char), and I checked against the stdlib we build with (2.1.20) that a no-op String.replace returns the receiver itself rather than a copy:
String s = "hello world";
String r = kotlin.text.StringsKt.replace(s, "\u0000", "", false); // identity == trueSo the alternative would be to always call replace and derive changed from sanitized !== value.value. I avoided that because returning this is an implementation detail of the stdlib rather than a documented contract, and the explicit check reads more obviously. If you'd prefer the shorter version (or an indexOf result reused for the replace), say so and I'll push it.
There was a problem hiding this comment.
what if we compare the length and not the whole value?
There was a problem hiding this comment.
👍 On it. Length comparison works and is cheaper than the pre-scan — removing NULs is the only edit, so the length differs exactly when something was stripped. Switching to a single replace plus sanitized.length != value.value.length for the changed flag, and I'll re-run the connector unit tests before pushing.
There was a problem hiding this comment.
☑️ Resolved in dbf87a0. Single replace now, with changed derived from sanitizedValue.length != value.value.length; the pre-scan is gone. Connector unit tests still pass locally (:airbyte-integrations:connectors:destination-postgres:test — BUILD SUCCESSFUL) and spotless is clean.
Co-Authored-By: bot_apk <apk@cognition.ai>
| // Sanitize null bytes from JSON data — PostgreSQL TEXT columns do not support \u0000 | ||
| val jsonData = Jsons.writeValueAsString(filteredRecord).replace("\u0000", "") |
There was a problem hiding this comment.
why didn't this work before?
There was a problem hiding this comment.
🚫 No change needed — answering. It ran too late. By the time Jsons.writeValueAsString(...) returns, Jackson has already encoded the NUL as the six-character escape sequence \u0000 (backslash, u, 0000), so there is no literal NUL code point left in the string for .replace("\u0000", "") to match. The replace silently matched nothing on every record.
Quick demonstration with the Jackson version we build against:
input: {"a": "x<NUL>y"} // actual U+0000 in the value
output: {"a":"x\u0000y"} // six literal characters, zero NUL bytes
And \u0000 in that form is precisely what Postgres' jsonb input function rejects (unsupported Unicode escape sequence / <NULL> cannot be converted to text), which is the failure in the linked issue. Sanitizing the AirbyteValue tree before serialization is what makes it effective, and the new PostgresRecordFormatterTest asserts the absence of both the NUL byte and the textual escape so this can't silently regress to a no-op again.
There was a problem hiding this comment.
shouldn't there be a way to use replace with the string value instead of the byte value?
There was a problem hiding this comment.
🙋 There is, but I'd rather not — your call. You're right that .replace("\\u0000", "") on the serialized text would actually match, unlike the old code. Two reasons I went the other way:
-
It can corrupt valid JSON. If a source string legitimately contains the six characters
\u0000as text (backslash included), Jackson escapes the backslash and emits seven characters:\\u0000. A textual replace of\u0000matches the tail of that and leaves a dangling\, producing invalid JSON. Operating on theAirbyteValuetree has no such ambiguity — we edit the actual string content, and Jackson escapes whatever remains correctly. -
It only covers the raw path. The typed path doesn't serialize the whole record; each
jsonbcolumn is serialized independently (PostgresValueCoercer.map→toCsvValue) and the result is written into a CSV row. A post-serialization fixup would have to be repeated there, on strings already mixed with CSV quoting. Sanitizing before serialization is one place that covers both paths and keeps the typed and raw tables consistent.
If you'd still prefer the textual replace on the raw path, I'll switch it — but I'd want to keep the pre-serialization sanitizer for the typed path regardless, so it would be two mechanisms instead of one.
|
↪️ Triggering Reason: A new commit ( |
Reviewing PR for connector safety and quality.
|
🛡️ AI PR Review Report🟢 Review Action: APPROVED
All other gates passed. 🔶 Risk Level: 3/5Logic change in the per-record write path of an existing connector ( 📋 PR DetailsConnector(s): 🔍 Gate Evaluation DetailsGate-by-Gate Analysis
Detail notesLive / E2E — prove-fix SHA vs. current HEAD. The prove-fix conclusion was published for Per-Record Performance (WARNING, non-blocking). On the raw path, 📚 Evidence ConsultedEvidence
|
|
↪️ Triggering Reason: the refreshed |
Auto-merge evaluation: FAILEligibility (all must pass)
|
3 participants
What
Resolves https://github.com/airbytehq/oncall/issues/13292:
A sync on destination-postgres 3.0.16 failed during
COPYwith:The failing column is a semistructured column mapped to
jsonb, and Postgres'jsonbinput function rejects\u0000because it cannot be represented in Postgres text. The source record contained a NUL nested deep inside a JSON structure.NUL sanitization only covered top-level
StringValue(inPostgresValueCoercer.validate). Anything nested insideObjectValue/ArrayValuewas serialized untouched byAirbyteValueToCsvRow.toCsvValue(), so Jackson emitted the textual\u0000escape straight into the CSV stream feeding ajsonbcolumn.The raw-path sanitizer added in #75902 was also ineffective:
Jsons.writeValueAsString(record).replace("\u0000", "")replaces the actual NUL code point after Jackson has already escaped it as the six-character text\u0000, so nothing matched.How
Sanitization now happens on the
AirbyteValuetree, before any JSON serialization:sanitizePostgresValuewalksObjectValue/ArrayValuerecursively and strips\u0000from every nestedStringValue, returning the original instance when nothing changed (this is a per-record hot path). Values only, not object keys — consistent with the existingremoveNullCharactersexpectation mappers insrc/test-integration.PostgresValueCoercer.map()sanitizes before theUnionType/UnknownTypeserializeToString()step, so serialization can no longer produce a\u0000escape.validate()is now range/length validation only.PostgresRawRecordFormattersanitizes values beforeJsons.writeValueAsStringand drops the ineffective post-serializationreplace. This path is kept sanitizing independently of the coercer becausePostgresInsertBuffercan also be populated without going through the normal coercion pipeline.The fix is intentionally connector-local: the bulk-load CDK
ValueCoercerinterface is destination-specific, so no Postgres-specific behavior was added to the shared CDK.Stripping stays silent (no meta change / nullification), matching the connector's pre-existing behavior for top-level strings and the test-integration expected-record mappers.
Review guide
write/transform/PostgresValueSanitizer.kt— new recursive sanitizerwrite/transform/PostgresValueCoercer.kt— sanitize inmap(), remove strip fromvalidate()write/load/PostgresRecordFormatter.kt— sanitize before serialization on the raw pathsrc/test/.../PostgresRecordFormatterTest.kt,PostgresValueCoercerTest.ktTest Coverage
New tests exercise the full serialize path and assert the absence of the textual
\u0000escape (asserting only the absence of an actual NUL passes vacuously, which is why the previous formatter test did not catch this):PostgresValueCoercerTest.testMapRemovesNestedNullCharacters— NUL nested in anObjectValueand inside anArrayValueis gone aftermap(), asserted on the value itself.PostgresRecordFormatterTest— a nested NUL routed throughPostgresValueCoercer.map+PostgresSchemaRecordFormatter(jsonb column) and +PostgresRawRecordFormatter(_airbyte_data) produces output containing neither an actual NUL nor the\u0000escape.Verified each new test fails against the unmodified sources (3 failures) and passes with the fix; full connector unit suite
./gradlew :airbyte-integrations:connectors:destination-postgres:testis green.Breaking change evaluation
Not breaking: no schema, spec, stream, PK/cursor or state change. Records that previously failed the whole
COPYnow land with NUL characters removed, which is the behavior already applied to top-level string fields.enableProgressiveRollout: true, so the version is3.0.17-rc.1.User Impact
Syncs no longer fail with
unsupported Unicode escape sequencewhen source JSON contains NUL characters nested inside objects or arrays; those characters are removed from the written value.Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/41a50ff04f8f40978fff6d97f17cd4cb