Repository navigation
feat: add java.sql.Clob converter - #1134
AhmedYoussef98 wants to merge 1 commit into
Conversation
| } | ||
|
|
||
| @Test | ||
| void convertsClobCharacterStreamToCellString() throws Exception { |
There was a problem hiding this comment.
Is there a test gap on the write path? convertsClobCharacterStreamToCellString uses a 13-char value, so the 4096-char read loop only runs once. Changing while to if (so only the first chunk is kept) still passes all three tests. Using the cell text limit as the length covers several chunks and delei's boundary at the same time:
@Test
void convertsClobAtCellTextLimitToCellString() throws Exception {
StringBuilder expected = new StringBuilder();
for (int i = 0; i < 32767; i++) {
expected.append((char) ('a' + i % 26));
}
Clob value = new SerialClob(expected.toString().toCharArray());
WriteCellData<?> written = converter.convertToExcelData(value, null, GLOBAL_CONFIGURATION);
Assertions.assertEquals(expected.toString(), written.getStringValue());
}There was a problem hiding this comment.
Added convertsClobAtCellTextLimitToCellString testing through 32,767 characters to verify chunking across buffer boundaries up to the Excel cell text limit. Thanks!
|
Hi @AhmedYoussef98 — checking in on this one. It has been quiet since late September, and the branch currently has conflicts with main. Are you still planning to carry it forward? If you don't have the bandwidth for it right now, I'd be happy to pick it up — I'd rebase the converter on current main, keep the implementation and tests, adjust to the current testkit conventions where needed, and reference this PR and #1017 in the description so your work stays credited. Either way, just let me know. |
cc27926 to
e26ef61
Compare
|
Hi @Mikkey-f, thanks for checking in! I've just rebased onto |
|
Great to hear — thanks for the quick turnaround on the rebase. I'll leave it with you then; withdrawing my offer. |
| StringBuilder text = new StringBuilder(); | ||
| char[] buffer = new char[4096]; | ||
| try (Reader reader = value.getCharacterStream()) { | ||
| int count; | ||
| while ((count = reader.read(buffer)) != -1) { | ||
| text.append(buffer, 0, count); | ||
| } | ||
| } | ||
| return new WriteCellData<>(text.toString()); |
There was a problem hiding this comment.
I'd suggest pre-sizing the StringBuilder and adding a length check. This avoids the overhead of multiple array expansions for large payloads and prevents potential integer overflow when casting clob.length() (which is a long) to int.
long length = value.length();
if (length == 0) {
return new WriteCellData<>(CellDataTypeEnum.EMPTY);
}
if (length > Integer.MAX_VALUE) {
throw new IllegalArgumentException(String.format("Clob length (%d) exceeds maximum allowed: %d", length, Integer.MAX_VALUE));
}
StringBuilder text = new StringBuilder((int) length);| if (cellData.getStringValue() == null) { | ||
| return null; | ||
| } | ||
| // Materialize cell text into a detached JDBC Clob, avoiding a database-specific Clob implementation. |
There was a problem hiding this comment.
I believe the code itself already expresses its intent clearly, so there is no need to add extra comments.
This branch has not been deployed
5 participants
Purpose of the pull request
Related to #1017
What's changed?
Adds a default
java.sql.Clobconverter. Clob values are written to Excel as string cells by reading their character stream. String cells are converted back to detachedSerialClobinstances, avoiding database-specific implementations.The converter is registered for default reads and writes. Unit tests cover converter keys, both conversion directions, and loader registration.
Verification
./mvnw package -B -Dmaven.test.skip=false -Dtest=ClobStringConverterTest,DefaultConverterLoaderTest -Dsurefire.failIfNoSpecifiedTests=false -pl fesod-common,fesod-shaded,fesod-sheet— 8 tests passed../mvnw spotless:check -B— passed.The full spreadsheet suite did not complete on macOS because
CompatibilityTest.readWithCacheAfterTempDeletionrecursed repeatedly throughFileUtils.delete; the targeted converter and loader tests passed.Checklist