Skip to content

Commit b1d35a6

Browse files
nkuprinsdelei
andauthored
fix: descriptive error for unrecognized cell type attribute (#965)
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 #955 Co-authored-by: DeleiGuo <delei@apache.org>
1 parent b912bea commit b1d35a6

3 files changed

Lines changed: 72 additions & 5 deletions

File tree

fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.apache.fesod.sheet.constant.FesodSheetConstants;
3434
import org.apache.fesod.sheet.context.xlsx.XlsxReadContext;
3535
import org.apache.fesod.sheet.enums.CellDataTypeEnum;
36+
import org.apache.fesod.sheet.exception.ExcelAnalysisException;
3637
import org.apache.fesod.sheet.metadata.GlobalConfiguration;
3738
import org.apache.fesod.sheet.metadata.data.ReadCellData;
3839
import org.apache.fesod.sheet.read.metadata.holder.xlsx.XlsxReadSheetHolder;
@@ -49,8 +50,8 @@ public class CellTagHandler extends AbstractXlsxTagHandler {
4950
@Override
5051
public void startElement(XlsxReadContext xlsxReadContext, String name, Attributes attributes) {
5152
XlsxReadSheetHolder xlsxReadSheetHolder = xlsxReadContext.xlsxReadSheetHolder();
52-
xlsxReadSheetHolder.setColumnIndex(PositionUtils.getCol(
53-
attributes.getValue(ExcelXmlConstants.ATTRIBUTE_R), xlsxReadSheetHolder.getColumnIndex()));
53+
String cellReference = attributes.getValue(ExcelXmlConstants.ATTRIBUTE_R);
54+
xlsxReadSheetHolder.setColumnIndex(PositionUtils.getCol(cellReference, xlsxReadSheetHolder.getColumnIndex()));
5455

5556
// t="s" ,it means String
5657
// t="str" ,it means String,but does not need to be read in the 'sharedStrings.xml'
@@ -59,7 +60,11 @@ public void startElement(XlsxReadContext xlsxReadContext, String name, Attribute
5960
// t="e" ,it means Error
6061
// t="n" ,it means Number
6162
// t is null ,it means Empty or Number
62-
CellDataTypeEnum type = CellDataTypeEnum.buildFromCellType(attributes.getValue(ExcelXmlConstants.ATTRIBUTE_T));
63+
String cellType = attributes.getValue(ExcelXmlConstants.ATTRIBUTE_T);
64+
CellDataTypeEnum type = CellDataTypeEnum.buildFromCellType(cellType);
65+
if (type == null) {
66+
throw new ExcelAnalysisException("Invalid cell data type: '" + cellType + "' in cell " + cellReference);
67+
}
6368
xlsxReadSheetHolder.setTempCellData(new ReadCellData<>(type));
6469
xlsxReadSheetHolder.setTempData(new StringBuilder());
6570

fesod-sheet/src/main/java/org/apache/fesod/sheet/enums/CellDataTypeEnum.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,9 @@ public enum CellDataTypeEnum {
8383
/**
8484
* Build data types
8585
*
86-
* @param cellType
87-
* @return
86+
* @param cellType the raw {@code t} attribute value of a cell
87+
* @return the matching type, {@link #EMPTY} when {@code cellType} is empty, or {@code null} when
88+
* {@code cellType} is not a recognized type; callers are expected to handle the {@code null} case.
8889
*/
8990
public static CellDataTypeEnum buildFromCellType(String cellType) {
9091
if (StringUtils.isEmpty(cellType)) {
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.fesod.sheet.analysis.v07.handlers;
21+
22+
import org.apache.fesod.sheet.context.xlsx.XlsxReadContext;
23+
import org.apache.fesod.sheet.exception.ExcelAnalysisException;
24+
import org.apache.fesod.sheet.read.metadata.holder.xlsx.XlsxReadSheetHolder;
25+
import org.apache.fesod.sheet.testkit.Tags;
26+
import org.junit.jupiter.api.Assertions;
27+
import org.junit.jupiter.api.Tag;
28+
import org.junit.jupiter.api.Test;
29+
import org.mockito.Mockito;
30+
import org.xml.sax.helpers.AttributesImpl;
31+
32+
/**
33+
* Regression test for <a href="https://github.com/apache/fesod/issues/955">issue #955</a>.
34+
*
35+
* <p>A cell whose {@code t} attribute is not a recognized type made {@code buildFromCellType} return
36+
* {@code null}, which then tripped the {@code ReadCellData} constructor with a confusing
37+
* {@code IllegalArgumentException: Type can not be null} that named neither the cell nor the attribute.
38+
* The handler must instead throw an {@link ExcelAnalysisException} naming the invalid type and its location.
39+
*/
40+
@Tag(Tags.UNIT)
41+
class CellTagHandlerTest {
42+
43+
@Test
44+
void startElement_throwsDescriptiveError_forUnknownCellType() {
45+
XlsxReadContext context = Mockito.mock(XlsxReadContext.class);
46+
XlsxReadSheetHolder sheetHolder = Mockito.mock(XlsxReadSheetHolder.class);
47+
Mockito.when(context.xlsxReadSheetHolder()).thenReturn(sheetHolder);
48+
49+
AttributesImpl attributes = new AttributesImpl();
50+
attributes.addAttribute("", "r", "r", "CDATA", "B4");
51+
attributes.addAttribute("", "t", "t", "CDATA", "unknown");
52+
53+
ExcelAnalysisException exception = Assertions.assertThrows(
54+
ExcelAnalysisException.class, () -> new CellTagHandler().startElement(context, "c", attributes));
55+
56+
// The message must name the unrecognized type and the exact cell (Excel reference) for diagnostics.
57+
String message = exception.getMessage();
58+
Assertions.assertTrue(message.contains("'unknown'"), "should name the unrecognized type: " + message);
59+
Assertions.assertTrue(message.contains("B4"), "should name the cell reference: " + message);
60+
}
61+
}

0 commit comments

Comments
 (0)