GeezerVison is showing github.com — Support for JSON output with `index` properties by mihaibudiu · Pull Request #7752 · feldera/feldera
Conversation
Linked Issue
Fixes #7723
Fixes #7750
There are 4 separate commits, which should work independently.
Checklist
- Unit tests added/updated
- Documentation updated
- Changelog updated
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Strix Security Review
✅ No open security findings.
PR summary: Adds a JSON output encoder path for connectors attached to a view index: Debezium-format change events keyed by the index columns, `u` events with `before`/`after`, optional tombstones after deletes (`tombstones_on_delete`), the Debezium value schema now declaring `before`, CSV rejecting `index`, and a refactor of the profiler support-bundle SQL panel.
Review summary
Reviewed the new indexed JSON/Debezium encoder path (encode_indexed, JsonEncoder::new, the new validate rules), the rewritten buffer-size accounting in the non-indexed encode loop, the new tombstones_on_delete config (Rust and Java), the Debezium schema change, the CSV index rejection, and the profiler frontend refactor. No vulnerability was confirmed: the message-size accounting in both the indexed and non-indexed paths is exact or fail-closed (and now includes key bytes, closing a pre-existing over-limit case), the index/key_schema pairing is structurally guaranteed by the catalog, attacker-controlled records cannot multiply per-key output or panic the error formatter, and the SQL panel renders bundle content as escaped text with no raw-HTML sink. Remaining observations are author-only configuration and correctness nits that cross no trust boundary.
Ruled out (6)
- Can the new indexed encoder path be made to overflow the transport's max-buffer-size limit or panic on attacker-controlled data?
The `required` size check equals the bytes actually pushed for both array modes, per-key output is bounded to one `before` and one `after` by `indexed_operation_type`, and the error formatter is UTF-8-safe and length-bounded.
- Does the rewritten non-indexed buffer accounting drop records, underflow, or let a keyed message exceed the transport limit?
The rewrite is purely additive with no subtraction left, `prev_len` truncation is always in range, deferred records keep their weight and sequence number, and including `key_buffer` makes the guard stricter than before.
- Can a connector config reach the index encoder in an unsafe or unsupported state, or bypass transport capability checks?
`is_index` is derived from the same condition that forces `index_handles` resolution, which only yields handles with a key schema, and the residual mismatches fail the author's own endpoint at runtime rather than crossing any security boundary.
- Does the support-bundle viewer render bundle-supplied SQL or config through an unescaped HTML sink, or let one user's bundle break another user's session?
No `{@html}` sink exists on the bundle path, Monaco `setValue` is a plain text buffer, the new `sqlPanel` snippet has no production consumer, and bundle parsing is entirely client-side.
- Is the unguarded `program_code.split('\n')` in the bundle parser a reportable availability issue?
Server-generated bundles always carry a string `program_code`, and the TypeError is caught by the load guard and shown as escaped text in the opener's own tab only.
- Should `array: true` combined with an index be treated as a security issue?
It only affects the output shape of a sink the author configured themselves, is the documented meaning of the `array` option, and has no size-accounting or memory-safety consequence.
Review 1 on this PR · 8c9fc52 · requested Oct 30 20:31 UTC when the PR was opened
Reviewed by Strix
Re-run review · Configure security review settings
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Review: JSON Debezium output for index, CSV index rejection, buffer-size fix, before in the Kafka Connect schema
What I ran:
| Check |
Result |
cargo test -p static-connectors for format::json::output, format::csv::test, and the aliased-index controller::test cases |
35/35 pass |
cargo test -p dbsp_connectors kafka_connect_json_converter |
pass |
mvn test -Dtest=ConnectorTests |
161/161 pass |
pre-commit run on the changed files |
pass |
QA: new test for array: true with an index (in the inline comment) |
pass |
The Kafka tests in the same filter failed only because no broker was running in my environment, so that failure is not from this PR.
Gates:
- SQL type coverage (medium): this changes format/encoding, but the indexed path is tested only on the 4-column
Basic relation with a single BIGINT key. Please add an every-type round-trip (see inline comment).
- Breaking changes (CSV +
index is now rejected; the Debezium value schema gains before) are documented in the changelog. User docs and the config rustdoc are updated. No new dependencies and no unsafe.
Cases the PR's tests don't cover:
- missing (medium): a composite index key, or a key with a nullable, temporal, or DECIMAL column.
- missing (low):
array: true combined with an index.
- pitfall (medium): the compiler accepts
tombstones_on_delete without an index, so the error only appears at runtime (see inline comment).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with sample data locally and everything worked well. Thank you!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…ormat
With an index, the JSON encoder writes one Debezium event for each key
that a step changes: `c` with `after` for an insert, `u` with `before`
and `after` for an update, and `d` with `before` for a delete. The
message key holds the index columns. A tombstone follows each `d`,
unless `tombstones_on_delete` is false. The output without an index
does not change.
Fixes #7723
Signed-off-by: Mihai Budiu <[email protected]>
Before, the CSV encoder ignored the index and wrote each update as a
delete and an insert. The controller tests of aliased indexes now
attach JSON Debezium output to the index, on an in-memory endpoint
that keeps keyed messages.
Signed-off-by: Mihai Budiu <[email protected]>
The size check of the JSON encoder did not count the message key, the
`\n` after the last record, or the `]\n` after an array. When a record
did not fit, the encoder wrote `\n` again, so the message ended in an
empty line. A message could be larger than the transport's limit by
the size of its key plus two bytes.
Signed-off-by: Mihai Budiu <[email protected]>
… JSON output
A Debezium delete event holds the deleted row in
`before`, but the schema that the `kafka_connect_json_converter` flavor
writes declared only `after` and `op`. The Kafka Connect JsonConverter
builds its record from the schema, so a delete reached Kafka Connect
without the row. The value schema now declares `before`, as the
Debezium envelope does for every event.
Fixes #7750
Signed-off-by: Mihai Budiu <[email protected]>
A Kafka output connector with exactly-once fault tolerance does not
write tombstones. JSON `debezium` output with an index writes a
tombstone after each `d` event, unless `tombstones_on_delete` is false,
so such a pipeline must set it to false.
Signed-off-by: Mihai Budiu <[email protected]>
Since exactly-once Kafka output accepts keyed messages, Avro output
with an index works in that mode. Only the `confluent_jdbc` update
format cannot write its deletions, which are tombstones.
Signed-off-by: Mihai Budiu <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
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.
|