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

[nkuprins]

Search before asking

• I searched in the issues and found nothing similar.

Motivation

This is about failing with a useful error, not about supporting more cell types.

A cell whose t attribute is not one of s, str, inlineStr, e, b, n aborts the whole read with an error that names neither the cell nor the attribute:

java.lang.IllegalArgumentException: Type can not be null
    at org.apache.fesod.sheet.metadata.data.ReadCellData.<init>(ReadCellData.java:78)
    at org.apache.fesod.sheet.analysis.v07.handlers.CellTagHandler.startElement(CellTagHandler.java:63)

Repro:
Any xlsx sheet that contains e.g. <c r="A1" t="d"><v>2024-05-01T00:00:00</v></c> (or t="anything")

Cause:
In CellDataTypeEnum.buildFromCellType, a missing t attribute is handled - it returns EMPTY. But when t is present with a value that isn't in the map, the lookup returns null and nothing checks for it. That null then trips the ReadCellData constructor, which is where the confusing exception comes from.

Solution

Keep failing, but say what happened. In CellTagHandler.startElement:

String cellType = attributes.getValue(ExcelXmlConstants.ATTRIBUTE_T);
CellDataTypeEnum type = CellDataTypeEnum.buildFromCellType(cellType);
if (type == null) {
    throw new ExcelAnalysisException("Unsupported cell type '" + cellType + "'");
}

Alternatives

Fall back to DIRECT_STRING when the type is null

Anything else?

No response

Are you willing to submit a PR?

• I'm willing to submit a PR!

[bengbengbalabalabeng]

Based on my understanding, if explicit validation of unknown t attribute is performed in the method CellDataTypeEnum.buildFromCellType and a clearer exception is thrown (e.g., throw new IllegalArgumentException("Invalid cell data type: xxx")), it would be more consistent with the fast-fail processing logic.

The reason for using IllegalArgumentException is that this is not a logical exception in the parsing flow; treating it as an argument error is more semantically appropriate here.

[nkuprins]

Based on my understanding, if explicit validation of unknown t attribute is performed in the method CellDataTypeEnum.buildFromCellType and a clearer exception is thrown (e.g., throw new IllegalArgumentException("Invalid cell data type: xxx")), it would be more consistent with the fast-fail processing logic.

The reason for using IllegalArgumentException is that this is not a logical exception in the parsing flow; treating it as an argument error is more semantically appropriate here.

@bengbengbalabalabeng thanks for taking a look! One thing I noticed is that throwing in CellTagHandler.startElement lets us point to the exact cell, which can be useful for diagnostics:

String cellType = attributes.getValue(ExcelXmlConstants.ATTRIBUTE_T);
CellDataTypeEnum type = CellDataTypeEnum.buildFromCellType(cellType);
if (type == null) {
    throw new ExcelAnalysisException( // or IllegalArgumentException
            "Invalid cell data type: '" + cellType + "' at row "
                    + xlsxReadSheetHolder.getRowIndex() + ", column " + xlsxReadSheetHolder.getColumnIndex());
}
xlsxReadSheetHolder.setTempCellData(new ReadCellData<>(type));

Would you say the clearer diagnostics (naming the exact row/column) outweigh keeping the validation centralised in buildFromCellType?

• If yes: which exception would you prefer there - ExcelAnalysisException or IllegalArgumentException?
• If no, then I think this is the version you have in mind:

public static CellDataTypeEnum buildFromCellType(String cellType) {
    if (StringUtils.isEmpty(cellType)) {
        return EMPTY;
    }
    CellDataTypeEnum type = TYPE_ROUTING_MAP.get(cellType);
    if (type == null) {
        throw new IllegalArgumentException("Invalid cell data type: " + cellType);
    }
    return type;
}

[bengbengbalabalabeng]

in CellTagHandler.startElement lets us point to the exact cell, which can be useful for diagnostics

+1

Additionally, it is recommended to add a comment inside CellDataTypeEnum.buildFromCellType to clearly state that null may be returned when the passed t attribute cannot be recognized.