From 09f076db7cadaa189c1aa174a4f2e4d791158d55 Mon Sep 17 00:00:00 2001 From: ian zhang Date: Sun, 19 Jul 2026 08:53:53 +0800 Subject: [PATCH 1/3] docs: add Copilot repository and code review instructions - .github/copilot-instructions.md: repository-wide instructions covering project overview, build/test commands, code style, module structure, testing conventions, and CI checks - .github/instructions/code-review.instructions.md: path-specific Java review instructions with 8 priority checks and a pitfalls table derived from real bug fixes (sql.Date toInstant, negative value validation, premature ThreadLocal cleanup, missing defensive copy, FileWriter charset) --- .github/copilot-instructions.md | 84 +++++++++++++++++++ .../instructions/code-review.instructions.md | 71 ++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 .github/copilot-instructions.md create mode 100644 .github/instructions/code-review.instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 000000000..b0a8a21b4 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,84 @@ +# Apache Fesod (Incubating) — Copilot Repository Instructions + +## Project Overview + +Apache Fesod (Incubating) is a Java library for processing spreadsheets (XLS/XLSX/CSV) without OOM, derived from Alibaba EasyExcel. It uses Apache POI under the hood with streaming SAX-based reading. The project is an Apache incubator project and must follow ASF conventions. + +- **Language**: Java 8+ (source/target = 1.8, but builds on JDK 17+) +- **Build**: Maven 3.6.3 via `./mvnw` wrapper (project docs require 3.9+, but wrapper currently pins 3.6.3; both work) +- **CI matrix**: JDK 8, 11, 17, 21, 25 + +## Build & Test Commands + +```bash +# Build (skips tests by default) +./mvnw clean install -DskipTests + +# Run tests (must explicitly enable) +./mvnw clean package -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet,fesod-examples/fesod-sheet-examples + +# Format check / auto-format +./mvnw spotless:check +./mvnw spotless:apply + +# Run tests by tag +./mvnw test -Dtest.groups=unit # fast unit tests only +./mvnw test -Dtest.excludedGroups=fuzz # exclude slow fuzz tests + +# Full install with javadoc (CI validation) +./mvnw install -B -V +./mvnw javadoc:javadoc +``` + +## Code Style & Conventions + +- **Formatter**: Palantir Java Format (enforced by Spotless, no checkstyle) +- **Imports**: Static imports first (`#|` pattern), then regular; no unused imports +- **Indent**: 4 spaces +- **License header**: ASF Apache 2.0 header required on all `.java`, `.xml`, `.yml`, `.toml` files (see `tools/spotless/license-header.txt`) +- **Lombok**: Allowed; config in `lombok.config` (`toString.callSuper = CALL`, `equalsAndHashCode.callSuper = CALL`) +- **Commits**: English, format `type: description` (types: `fix`, `feat`, `refactor`, `test`, `docs`, `chore`, `style`, `dependency`) + +## Module Structure + +``` +fesod-parent/ # Root POM, dependency & plugin management +├── fesod-common/ # Zero-dependency utilities (org.apache.fesod.common.util) +├── fesod-shaded/ # Relocated Spring ASM/cglib (org.apache.fesod.shaded) +├── fesod-bom/ # BOM for downstream consumers +├── fesod-sheet/ # Core library: read/write XLS/XLSX/CSV via POI +│ ├── src/main/java/org/apache/fesod/sheet/ +│ │ ├── FesodSheet.java # Main entry: FesodSheet.read() / FesodSheet.write() +│ │ ├── analysis/ # Read pipeline (v03=XLS BIFF, v07=XLSX SAX, csv) +│ │ ├── write/ # Write pipeline (builder, executor, handler chains) +│ │ ├── metadata/ # Data models, builders, csv/ property/ +│ │ ├── converters/ # Type conversion framework (by Java type) +│ │ └── util/ # DateUtils, NumberUtils, WorkBookUtil, etc. +│ └── src/test/java/org/apache/fesod/sheet/ +│ └── testkit/ # Test infrastructure (NOT a separate module) +│ ├── Tags.java # @Tag constants: unit, round-trip, read, write, format, fuzz +│ ├── base/ # AbstractExcelTest (round-trip base) +│ ├── assertions/ # ExcelAssertions fluent API +│ └── builders/ # TestDataBuilder +└── fesod-examples/ # Usage examples +``` + +## Testing Conventions + +- **JUnit 5** with `@TestInstance(PER_CLASS)`, `@DisplayName(ReplaceUnderscores)` +- **Tags** (use `@Tag(Tags.XXX)`): + - `unit` — pure logic, no file I/O + - `round-trip` — write-then-read via `AbstractExcelTest` + - `read` / `write` — single-direction tests + - `format` — CSV/charset/date format tests + - `fuzz` — Jazzer property-based (slow, excluded in PR CI) +- **Real-file tests**: Prefer `AbstractExcelTest.createTempFile()` + `ExcelAssertions` for integration tests +- **Test data**: Use `TestDataBuilder.simpleData(n)` or model classes in `testkit/models/` + +## CI Checks (must pass before merge) + +1. `spotless:check` — format +2. License header (hawkeye) +3. Multi-JDK test matrix (8/11/17/21/25) +4. `mvn install` + `mvn javadoc:javadoc` +5. CodeQL security scan diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md new file mode 100644 index 000000000..8c50e1ea3 --- /dev/null +++ b/.github/instructions/code-review.instructions.md @@ -0,0 +1,71 @@ +--- +applyTo: "**/*.java" +--- + +# Code Review Instructions for Apache Fesod + +## Priority Checks + +### 1. Java 8 Source Compatibility + +The project targets Java 8 (`source/target = 1.8`) but CI runs on JDK 8 through 25. Review for: + +- **No Java 9+ APIs**: Methods like `List.of()`, `Map.of()`, `String.strip()`, `InputStream.readAllBytes()` do not exist in Java 8. Use `Collections.singletonList()`, `new HashMap<>()`, `String.trim()`, or Apache Commons equivalents instead. +- **`java.sql.Date.toInstant()` / `java.sql.Time.toInstant()`**: These throw `UnsupportedOperationException` on Java 9+ because the types are date-only/time-only. When converting `java.util.Date` subclasses, always use `instanceof` checks and call `toLocalDate()` / `toLocalTime()` instead. This applies to both `WriteCellData` and `CsvCell` code paths — keep them consistent. +- **No `var` keyword**: Java 8 does not support `var`. Always declare explicit types. + +### 2. Immutability & Defensive Copies + +Builder methods that accept or return mutable collections must protect internal state: + +- **`head(Consumer)` pattern**: When `AbstractParameterBuilder.head(Consumer)` stores the result of `DefaultHeadBuilder.define()`, it must wrap it with `toMutableListIfNecessary()` to create a defensive copy. Otherwise, `ExcelHeadProperty.initHeadRowNumber()` will mutate the builder's internal list during write, corrupting subsequent reuses. +- **General rule**: Any builder method that stores a collection from an external source must copy it. Any getter that returns an internal collection should document whether it's modifiable. + +### 3. Input Validation & Boundary Values + +- **Negative values**: Validation logic must handle all invalid values, not just sentinel defaults. For example, `@FreezePane(leftmostColumn = -1)` uses `-1` as "use default", but `leftmostColumn = -5` must also fall back to default — check `value == null || value < 0`, not just `value == -1`. +- **Null safety**: Public API entry points must validate null inputs and fail fast with a clear `IllegalArgumentException` rather than NPE deep in the call stack. + +### 4. Resource & Lifecycle Management + +- **ThreadLocal cleanup**: When setting thread-local state (e.g., `Biff8EncryptionKey.setCurrentUserPassword()`), do not clear it prematurely in a `finally` block if the state is needed by a later phase. The cleanup must happen only after all consumers have finished — typically in a dedicated `clearXxx()` method called at the end of the overall operation. +- **Try-with-resources**: Always use try-with-resources for `ExcelWriter`, `ExcelReader`, `Workbook`, `OutputStream`, `Writer`. Never leave workbook resources unclosed. +- **File handles**: After writing Excel files via POI, ensure the workbook is fully closed before the file handle is released. + +### 5. API Consistency Across Code Paths + +The same operation may be implemented in multiple places. When fixing a bug in one path, check all parallel paths: + +- `WriteCellData` vs `CsvCell`: Both handle date conversion. A fix in one must be mirrored in the other. +- `FesodSheet` vs `FastExcel` vs `EasyExcel`: These are aliases. A behavior change in the modern API must not break legacy aliases. +- `ExcelWriterBuilder` vs `ExcelReaderBuilder`: Symmetric builder patterns should stay symmetric. + +### 6. Test Quality + +- **Encoding consistency**: When tests write files, always specify the charset explicitly. Use `Files.newBufferedWriter(path, StandardCharsets.UTF_8)` instead of `new FileWriter(file)` (platform-default charset). The test must match the encoding configured in the production code (e.g., `CsvWorkbook` uses UTF-8). +- **Deprecated APIs in tests**: Avoid `new java.util.Date(year-1900, month, day)` — use `Calendar` or `java.time` types converted via `Date.from()`. Deprecated constructors generate warnings that pollute build output. +- **Tag selection**: Use the correct `@Tag`: + - `@Tag(Tags.UNIT)` for pure logic (no file I/O) + - `@Tag(Tags.ROUND_TRIP)` for write-then-read integration tests + - `@Tag(Tags.FORMAT)` for CSV/charset/date format tests + - `@Tag(Tags.WRITE)` for write-only tests +- **Real-file verification**: For bug fixes that affect file output, add at least one test that writes a real file and reads it back (via `ExcelAssertions` or POI `WorkbookFactory`). Code-level assertions alone may miss integration issues. +- **Regression evidence**: When fixing a bug, verify the test actually fails without the fix (revert source, run test, confirm failure, restore fix). + +### 7. ASF & Project Conventions + +- **License header**: Every new `.java` file must have the ASF Apache 2.0 header (see `tools/spotless/license-header.txt`). Spotless will fail CI if missing. +- **Package naming**: All code must be under `org.apache.fesod.*`. Never use `com.alibaba.*` or `cn.idev.*` (legacy packages from EasyExcel era). +- **Lombok**: `toString.callSuper = CALL` and `equalsAndHashCode.callSuper = CALL` are enforced via `lombok.config`. Subclasses must call super. +- **No checked exceptions in public API**: Wrap checked exceptions in `ExcelGenerateException` or `ExcelAnalysisException` rather than declaring `throws` on builder methods. + +### 8. Common Pitfalls (from real bug fixes) + +| Pattern | Problem | Fix | +|---------|---------|-----| +| `value.toInstant()` on `java.sql.Date`/`Time` | `UnsupportedOperationException` on Java 9+ | Use `instanceof` + `toLocalDate()`/`toLocalTime()` | +| `value == -1` validation | Misses other negative values | Use `value == null \|\| value < 0` | +| `try { setThreadLocal(x); } finally { setThreadLocal(null); }` | Clears state needed by later phase | Remove premature cleanup; clean up at end of full operation | +| `parameter().setHead(DefaultHeadBuilder.define(c))` | Stores internal list reference | Wrap with `toMutableListIfNecessary()` | +| `new FileWriter(file)` in tests | Platform-default charset | Use `Files.newBufferedWriter(path, UTF_8)` | +| `new FileOutputStream(file)` outside try-with-resources | Resource leak on exception | Wrap in `try (FileOutputStream fos = new FileOutputStream(file)) { ... }` | From cf52aea36a4d26a7746d45144d713da9a1e15021 Mon Sep 17 00:00:00 2001 From: ian zhang Date: Sun, 19 Jul 2026 09:02:43 +0800 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .github/copilot-instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b0a8a21b4..6baefa9d7 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -4,7 +4,7 @@ Apache Fesod (Incubating) is a Java library for processing spreadsheets (XLS/XLSX/CSV) without OOM, derived from Alibaba EasyExcel. It uses Apache POI under the hood with streaming SAX-based reading. The project is an Apache incubator project and must follow ASF conventions. -- **Language**: Java 8+ (source/target = 1.8, but builds on JDK 17+) +- **Language**: Java 8+ (source/target = 1.8; CI builds/tests on JDK 8–25) - **Build**: Maven 3.6.3 via `./mvnw` wrapper (project docs require 3.9+, but wrapper currently pins 3.6.3; both work) - **CI matrix**: JDK 8, 11, 17, 21, 25 From b0983c29239c995d29453a9c39fb950d39031c79 Mon Sep 17 00:00:00 2001 From: ian zhang Date: Sun, 19 Jul 2026 09:38:09 +0800 Subject: [PATCH 3/3] docs: address Copilot review comments on PR #958 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copilot-instructions.md: - Fix misleading 'builds on JDK 17+' — clarify CI builds on JDK 8-25 - Remove unclear '(\#|\ pattern)' parenthetical, explain Spotless importOrder - Attribute license header enforcement to Hawkeye workflow, not Spotless - Replace non-existent 'fesod-parent/' dir with '(root)' in module tree - Fix '@DisplayName(ReplaceUnderscores)' to reference junit-platform.properties code-review.instructions.md: - Attribute license header enforcement to Hawkeye workflow, not Spotless - Fix backslash-escaped '\|\|' rendering issue in markdown table - Use 'StandardCharsets.UTF_8' instead of 'UTF_8' for consistency --- .github/copilot-instructions.md | 10 +++++----- .github/instructions/code-review.instructions.md | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 6baefa9d7..7348fb743 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -4,7 +4,7 @@ Apache Fesod (Incubating) is a Java library for processing spreadsheets (XLS/XLSX/CSV) without OOM, derived from Alibaba EasyExcel. It uses Apache POI under the hood with streaming SAX-based reading. The project is an Apache incubator project and must follow ASF conventions. -- **Language**: Java 8+ (source/target = 1.8; CI builds/tests on JDK 8–25) +- **Language**: Java 8+ (source/target = 1.8; CI builds on JDK 8, 11, 17, 21, 25 — JDK 17+ is recommended for local development) - **Build**: Maven 3.6.3 via `./mvnw` wrapper (project docs require 3.9+, but wrapper currently pins 3.6.3; both work) - **CI matrix**: JDK 8, 11, 17, 21, 25 @@ -33,16 +33,16 @@ Apache Fesod (Incubating) is a Java library for processing spreadsheets (XLS/XLS ## Code Style & Conventions - **Formatter**: Palantir Java Format (enforced by Spotless, no checkstyle) -- **Imports**: Static imports first (`#|` pattern), then regular; no unused imports +- **Imports**: Static imports first, then regular imports; no unused imports (enforced by Spotless `importOrder` with `\#|` separator) - **Indent**: 4 spaces -- **License header**: ASF Apache 2.0 header required on all `.java`, `.xml`, `.yml`, `.toml` files (see `tools/spotless/license-header.txt`) +- **License header**: ASF Apache 2.0 header required on all `.java`, `.xml`, `.yml`, `.toml` files. Header template at `tools/spotless/license-header.txt`; enforced by the Hawkeye workflow (`.github/workflows/license-check.yml`), not Spotless - **Lombok**: Allowed; config in `lombok.config` (`toString.callSuper = CALL`, `equalsAndHashCode.callSuper = CALL`) - **Commits**: English, format `type: description` (types: `fix`, `feat`, `refactor`, `test`, `docs`, `chore`, `style`, `dependency`) ## Module Structure ``` -fesod-parent/ # Root POM, dependency & plugin management +(root) # Root POM (artifactId: fesod-parent), dependency & plugin management ├── fesod-common/ # Zero-dependency utilities (org.apache.fesod.common.util) ├── fesod-shaded/ # Relocated Spring ASM/cglib (org.apache.fesod.shaded) ├── fesod-bom/ # BOM for downstream consumers @@ -65,7 +65,7 @@ fesod-parent/ # Root POM, dependency & plugin management ## Testing Conventions -- **JUnit 5** with `@TestInstance(PER_CLASS)`, `@DisplayName(ReplaceUnderscores)` +- **JUnit 5** with `PER_CLASS` lifecycle and `ReplaceUnderscores` display name generator (configured globally in `fesod-sheet/src/test/resources/junit-platform.properties`, not via per-class annotations) - **Tags** (use `@Tag(Tags.XXX)`): - `unit` — pure logic, no file I/O - `round-trip` — write-then-read via `AbstractExcelTest` diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 8c50e1ea3..52a9bf105 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -54,7 +54,7 @@ The same operation may be implemented in multiple places. When fixing a bug in o ### 7. ASF & Project Conventions -- **License header**: Every new `.java` file must have the ASF Apache 2.0 header (see `tools/spotless/license-header.txt`). Spotless will fail CI if missing. +- **License header**: Every new `.java` file must have the ASF Apache 2.0 header (see `tools/spotless/license-header.txt`). License headers are enforced by the Hawkeye workflow (`.github/workflows/license-check.yml`), not Spotless. - **Package naming**: All code must be under `org.apache.fesod.*`. Never use `com.alibaba.*` or `cn.idev.*` (legacy packages from EasyExcel era). - **Lombok**: `toString.callSuper = CALL` and `equalsAndHashCode.callSuper = CALL` are enforced via `lombok.config`. Subclasses must call super. - **No checked exceptions in public API**: Wrap checked exceptions in `ExcelGenerateException` or `ExcelAnalysisException` rather than declaring `throws` on builder methods. @@ -64,8 +64,8 @@ The same operation may be implemented in multiple places. When fixing a bug in o | Pattern | Problem | Fix | |---------|---------|-----| | `value.toInstant()` on `java.sql.Date`/`Time` | `UnsupportedOperationException` on Java 9+ | Use `instanceof` + `toLocalDate()`/`toLocalTime()` | -| `value == -1` validation | Misses other negative values | Use `value == null \|\| value < 0` | +| `value == -1` validation | Misses other negative values | Check `value == null` or `value < 0` | | `try { setThreadLocal(x); } finally { setThreadLocal(null); }` | Clears state needed by later phase | Remove premature cleanup; clean up at end of full operation | | `parameter().setHead(DefaultHeadBuilder.define(c))` | Stores internal list reference | Wrap with `toMutableListIfNecessary()` | -| `new FileWriter(file)` in tests | Platform-default charset | Use `Files.newBufferedWriter(path, UTF_8)` | +| `new FileWriter(file)` in tests | Platform-default charset | Use `Files.newBufferedWriter(path, StandardCharsets.UTF_8)` | | `new FileOutputStream(file)` outside try-with-resources | Resource leak on exception | Wrap in `try (FileOutputStream fos = new FileOutputStream(file)) { ... }` |