Skip to content

feat: add UUID string converter (#1017) - #1088

Open
zhanghuang090 wants to merge 5 commits into
apache:mainfrom
zhanghuang090:codex/add-uuid-string-converter
Open

zhanghuang090 wants to merge 5 commits into
apache:mainfrom
zhanghuang090:codex/add-uuid-string-converter

Conversation

@zhanghuang090

Copy link
Copy Markdown

Purpose of the pull request

Related: #1017

Fields declared as java.util.UUID currently have no built-in converter. This adds UUID ↔ STRING conversion so identifiers can be imported and exported without a custom converter.

Proposal: #1017 (comment)

What's changed?

  • Add UuidStringConverter, using UUID.fromString() for non-empty input and UUID.toString() for canonical lowercase output. Empty strings are treated as missing values, consistently with blank cells.
  • Register STRING reads, default writes, and explicit STRING writes in DefaultConverterLoader.
  • Add 18 JUnit 5 cases covering lookup paths, uppercase and boundary UUIDs, malformed input, and XLSX/XLS/CSV round trips, including null fields and empty strings. Malformed non-empty cells retain the framework's ExcelDataConvertException with the parsing error as the cause.
  • Keep the implementation compatible with Java 8 and add no dependencies.

Validation

  • Targeted UUID and existing loader tests: 22 passed (JDK 25.0.4.1, Maven 3.9.8).
  • Full module regression: 60 common tests passed; 934 spreadsheet tests had 0 assertion failures and 1 existing ClassUtilsFieldOverrideTest cleanup error (DirectoryNotEmptyException while deleting a JUnit temporary directory on Windows). A focused rerun of that class plus UUID and loader tests passed all 27 cases without code changes.
  • mvn -B -ntp -pl fesod-sheet -am -Dmaven.test.skip=false -Dtest=ClassUtilsFieldOverrideTest,UuidConverterTest,DefaultConverterLoaderTest -Dsurefire.failIfNoSpecifiedTests=false package spotless:check passed.
  • New converter compiles with javac --release 8 against the built project classes.
  • git diff --cached --check passed.

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

class UuidConverterTest extends AbstractExcelTest {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a @Tag(Tags.ROUND_TRIP)

UUID value = converter.convertToJavaData(new ReadCellData<>(input), null, configuration);
WriteCellData<?> cell = converter.convertToExcelData(value, null, configuration);
Assertions.assertEquals(CellDataTypeEnum.STRING, cell.getType());
Assertions.assertEquals(input.toLowerCase(java.util.Locale.ROOT), cell.getStringValue());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace the full java.util.Locale.ROOT in both places. Use import java.util.Locale;.

@nkuprins

Copy link
Copy Markdown
Contributor
  1. The test is named UuidConverterTest. Should we maybe rename it to uppercase UUID..., similar to the claim raised by bengbengbalabalabeng for UUIDStringConverter converter?
  2. Following up on my earlier @Tag(Tags.ROUND_TRIP) comment:
    Since unit tests and round-trip tests share one class, should we maybe split them?

One class with @Tag(Tags.UNIT) and another extending AbstractExcelTest with @Tag(Tags.ROUND_TRIP)

  1. In readsUppercaseAndBlankCells, the "padded UUID" and "whitespace UUID" rows never reach the converter's own trim/blank handling: the reader trims cell text by default (autoTrim), so the padded row arrives as TEXT, and the whitespace row arrives as an empty cell or as "". Removing .trim() from the converter still passes this test. Only trimsWhitespaceBeforeParsing catches it. Should we drop those two rows?

@nkuprins

Copy link
Copy Markdown
Contributor

Nit: #1032 keeps its registration checks in DefaultConverterLoaderTest (loadConvertersRegistersLocalTimeFamily), and #1092 adds a loadConvertersRegistersSqlTimeFamily next to it. If you'd like to follow that, the three DefaultConverterLoader assertions could move there as loadConvertersRegistersUuidFamily? The supportJavaTypeKey/supportExcelTypeKey assertions can stay here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants