Skip to content

fix: descriptive error for unrecognized cell type attribute#965

Open
nkuprins wants to merge 2 commits into
apache:mainfrom
nkuprins:fix/issue-955-descriptive-unknown-cell-type
Open

fix: descriptive error for unrecognized cell type attribute#965
nkuprins wants to merge 2 commits into
apache:mainfrom
nkuprins:fix/issue-955-descriptive-unknown-cell-type

Conversation

@nkuprins

Copy link
Copy Markdown

Purpose of the pull request

Closed: #955

What's changed?

An .xlsx cell whose t attribute isn't a recognized type (s, str, inlineStr, e, b, n) made CellDataTypeEnum.buildFromCellType return null, which CellTagHandler passed straight into new ReadCellData<>(type), leading to IllegalArgumentException: Type can not be null, which names neither the cell nor the attribute.

CellTagHandler.startElement now detects the null and throws an ExcelAnalysisException that names the type and the cell:

Invalid cell data type: 'unknown' in cell B4

Following the discussion with @bengbengbalabalabeng on the issue, the check lives in the handler (rather than centralised in CellDataTypeEnum.buildFromCellType), so the message can point at the exact cell. And CellDataTypeEnum.buildFromCellType Javadoc now documents that it may return null for an unrecognised type.

Covered by CellTagHandlerTest.

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.

An xlsx cell whose t attribute is not one of the recognized types
(s, str, inlineStr, e, b, n) caused CellDataTypeEnum.buildFromCellType
to return null. The very next line then tripped the ReadCellData
constructor with a confusing 'IllegalArgumentException: Type can not be
null' that named neither the cell nor the offending attribute.

Detect the null in CellTagHandler and throw an ExcelAnalysisException
that names the invalid type and the offending cell (by its Excel
reference, e.g. B4), and document the nullable return of
buildFromCellType so callers know to handle it. The read still aborts at
the same point; only the exception type and message change.

Closes apache#955

Copilot AI left a comment

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.

Pull request overview

This pull request improves diagnostics when reading .xlsx files by detecting unrecognized cell t attribute values during SAX parsing and throwing a descriptive ExcelAnalysisException that includes both the invalid type and the cell reference (addressing issue #955).

Changes:

  • Add a null-check in CellTagHandler.startElement for CellDataTypeEnum.buildFromCellType(...) and throw an ExcelAnalysisException with cell/type context when the type is unrecognized.
  • Document in CellDataTypeEnum.buildFromCellType Javadoc that the method may return null for unknown t values (and EMPTY for missing/empty t).
  • Add a regression unit test ensuring the thrown error message includes both the unknown type and the cell reference.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java Throws a descriptive ExcelAnalysisException when an unknown cell t value would otherwise lead to a null type and a confusing downstream exception.
fesod-sheet/src/main/java/org/apache/fesod/sheet/enums/CellDataTypeEnum.java Clarifies contract: may return null for unrecognized t values, EMPTY when t is empty/missing.
fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandlerTest.java Adds regression coverage verifying the new exception message includes both invalid type and cell reference.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

[Enhancement] Descriptive error for unrecognized cell type attribute instead of "IllegalArgumentException: Type can not be null"

2 participants