makssent commented on PR #39773: URL: https://github.com/apache/shardingsphere/pull/39773#issuecomment-5600789065
```markdown ### Result **Review Result: Not Mergeable** **Blocking Issues: 3** ### Blocking Issues #### 1. The new handle check is skipped for an empty batch **Evidence:** `FirebirdBatchBlobSegmentsCommandExecutor.java:51-57`. The PR adds a check of the `appendSegment()` result inside the loop. When `packet.getSegments()` is empty, the loop is skipped. For an unknown handle, `getBlobId()` returns an empty result, and a success response with `id=0` is sent. Firebird validates the handle before entering the loop: https://github.com/FirebirdSQL/firebird/blob/v5.0.3/src/remote/server/server.cpp#L5448-L5494 **Required Change:** validate the existence of the handle regardless of whether the request contains segments. Add tests for empty batches with unknown and closed handles. #### 2. The new OpenBlob check incorrectly rejects BLOB ID = 0 **Evidence:** `FirebirdOpenBlobCommandExecutor.java:46-49`. The PR replaces the previous empty-content fallback for IDs missing from the cache with `InvalidSegstrIdException`. For ID = 0, this turns a valid operation into an error. Firebird treats a zero ID as an empty BLOB: `blobId.isEmpty()` sets `BLB_eof` rather than raising `isc_bad_segstr_id`. https://github.com/FirebirdSQL/firebird/blob/v5.0.3/src/jrd/blb.cpp#L1373-L1377 **Relation to this PR:** the new check directly changes a previously successful empty-BLOB open into `invalid BLOB ID`. **Impact:** the client receives a BLOB error for a value that Firebird accepts. **Required Change:** preserve the valid handling of ID = 0 without weakening validation of genuinely unknown IDs. Add a dedicated test for opening a zero ID. #### 3. The new check reports an unknown-handle error for a valid read handle **Evidence:** `FirebirdPutBlobSegmentCommandExecutor.java:49-50`, `FirebirdBatchBlobSegmentsCommandExecutor.java:52-53`. After OpenBlob, a valid read handle exists. If a segment write is requested using that handle, `appendSegment()` does not find it in the write cache and returns an empty result. The check added by this PR interprets this as an unknown handle and raises `isc_bad_segstr_handle`. In Firebird, this handle passes the existence check. The error occurs later, when the write is attempted. For Firebird 4.0.6 and 5.0.3, the error is: - GDS: `isc_cannot_update_old_blob`; - code: `335545262`; - SQLSTATE: `42000`; - message: `cannot update old BLOB`. https://github.com/FirebirdSQL/firebird/blob/v5.0.3/src/jrd/blb.cpp#L1604-L1607 **Relation to this PR:** this finding concerns the exception selected by the new check, not the implementation of BLOB reading or storage. **Impact:** an existing handle used for an invalid operation is incorrectly classified as unknown. The same issue affects non-empty batches. **Required Change:** distinguish an unknown handle from a valid handle opened for reading. Verify the corresponding server error for the target Firebird version. ### Evidence Tests for an unknown BLOB handle and an unknown BLOB ID were written and passed: errors returned directly by Firebird and through ShardingSphere Proxy matched in SQLSTATE, GDS code, and message. Additional manual verification was performed in the debugger: replacing the BLOB handle before writing and the BLOB ID before opening caused Proxy to return the expected errors. | Error | GDS code | SQLSTATE | Message | |---|---|---|---| | Unknown BLOB handle | `335544328`, `isc_bad_segstr_handle` | `42000` | `invalid BLOB handle` | | Invalid BLOB ID | `335544329`, `isc_bad_segstr_id` | `42000` | `invalid BLOB ID` | The basic scenarios are therefore confirmed by tests and manual verification. The added vendor codes and messages are correct. The findings concern validation completeness and error selection in specific edge cases. These cases were analyzed against the source code but have not yet been reproduced separately against the database. ### Coverage - Reviewed the diff and stated scope of PR #39773 at head `951391bca0d`. - Reference implementations: Firebird 5.0.3 and Jaybird 5.0.6 source code. ``` -- 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]
