rishi-rana commented on PR #39773: URL: https://github.com/apache/shardingsphere/pull/39773#issuecomment-5611194858
Thanks for the detailed review — the Firebird source references made these easy to verify. All three blocking issues are fixed in 9b5f8a5. **1. Handle check skipped for an empty batch.** `FirebirdBatchBlobSegmentsCommandExecutor` now validates the handle before iterating segments, so an empty batch on an unknown or closed handle raises the error instead of returning `id=0` with success — matching `rem_port::put_segment()`. **2. BLOB ID = 0 incorrectly rejected.** `FirebirdOpenBlobCommandExecutor` now treats a zero id as the NULL BLOB quad and opens an empty BLOB, per `blb::open2()`. This also makes it consistent with `FirebirdExecuteStatementCommandExecutor.bindBlobParameters()`, which already had a `0L == blobId` case. Added `assertExecuteWithZeroBlobId`, and noted the convention in the class Javadoc so it doesn't read as a swallowed error. **3. Valid read handle reported as unknown.** Added `CannotUpdateOldBlobException` -> `isc_cannot_update_old_blob` (335545262 / 42000 / `cannot update old BLOB`, confirmed against Jaybird's `isc_error_sqlstates.properties` and `isc_error_msg.properties`), and a shared `FirebirdBlobWriteHandleValidator` used by both put and batch. One implementation note on 3 that's worth flagging: my first attempt detected the read handle via `FirebirdBlobReadCache`, but `readSegment()` removes the cursor once the BLOB is fully consumed, so a drained-but-unclosed read handle would still have reported `isc_bad_segstr_handle` — the same handle giving two different errors depending on how much had been read. The validator now checks allocation state in `FirebirdBlobHandleGenerator` instead, which is unaffected by read progress and is released by close/cancel (so a closed handle still correctly reports `isc_bad_segstr_handle`). `assertExecuteWithFullyReadHandle` covers this and fails against the cursor-based version. Also added tests for the empty-batch and read-handle paths, direct tests for the new `isAllocated`, and removed two tests that became byte-identical duplicates once validation moved ahead of the loop — with the check now preceding segment iteration, `assertExecuteWithUnknownBlobHandle` / `assertExecuteAfterClose` already exercise the empty-batch path, since the packet mock yields no segments. One item I deliberately did not change, in case you'd prefer otherwise: `appendSegment()`'s `OptionalInt` result is no longer consumed, since the up-front validation makes the empty case unreachable on a connection's serialized command path. Re-adding a post-append `checkState` would guard a `close_blob` interleaving mid-write, but it looked like duplicate validation of an invariant the validator already guarantees. Happy to add it back if you think the race is worth defending. Local verification: 311 tests across `database/exception/dialect/firebird` and `proxy/frontend/dialect/firebird`, zero failures; Checkstyle and Spotless clean under `-Pcheck`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
