Skip to content

Commit eacfa6e

Browse files
xurixuri
committed
Fix code review issues
1 parent cdd995f commit eacfa6e

File tree

8 files changed

+62
-111
lines changed

8 files changed

+62
-111
lines changed

‎calc.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"sync"
3030
"time"
3131
"unicode"
32+
"unicode/utf16"
3233
"unicode/utf8"
3334
"unsafe"
3435

@@ -14366,7 +14367,7 @@ func (fn *formulaFuncs) TEXTJOIN(argsList *list.List) formulaArg {
1436614367
return ok
1436714368
}
1436814369
result := strings.Join(args, delimiter.Value())
14369-
if utf16UnitCountInString(result) > TotalCellChars {
14370+
if len(utf16.Encode([]rune(result))) > TotalCellChars {
1437014371
return newErrorFormulaArg(formulaErrorVALUE, fmt.Sprintf("TEXTJOIN function exceeds %d characters", TotalCellChars))
1437114372
}
1437214373
return newStringFormulaArg(result)

‎calc_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3969,7 +3969,7 @@ func TestCalcCellValue(t *testing.T) {
39693969
"TEXTJOIN(\"\",TRUE,NA())": {"#N/A", "#N/A"},
39703970
"TEXTJOIN(\"\",TRUE," + strings.Repeat("0,", 250) + ",0)": {"#VALUE!", "TEXTJOIN accepts at most 252 arguments"},
39713971
"TEXTJOIN(\",\",FALSE,REPT(\"*\",32768))": {"#VALUE!", "TEXTJOIN function exceeds 32767 characters"},
3972-
"TEXTJOIN(\"\",FALSE,REPT(\"😀\",16384))": {"#VALUE!", "TEXTJOIN function exceeds 32767 characters"},
3972+
"TEXTJOIN(\"\",FALSE,REPT(\"\U0001F600\",16384))": {"#VALUE!", "TEXTJOIN function exceeds 32767 characters"},
39733973
// TRIM
39743974
"TRIM()": {"#VALUE!", "TRIM requires 1 argument"},
39753975
"TRIM(1,2)": {"#VALUE!", "TRIM requires 1 argument"},

‎cell.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"strconv"
2222
"strings"
2323
"time"
24+
"unicode/utf16"
2425

2526
"github.com/xuri/efp"
2627
)
@@ -446,7 +447,9 @@ func (f *File) SetCellStr(sheet, cell, value string) error {
446447

447448
// setCellString provides a function to set string type to shared string table.
448449
func (f *File) setCellString(value string) (t, v string, err error) {
449-
value = truncateUTF16Units(value, TotalCellChars)
450+
if len(utf16.Encode([]rune(value))) > TotalCellChars {
451+
value = truncateUTF16Units(value, TotalCellChars)
452+
}
450453
t = "s"
451454
var si int
452455
if si, err = f.setSharedString(value); err != nil {
@@ -507,7 +510,9 @@ func (f *File) setSharedString(val string) (int, error) {
507510

508511
// trimCellValue provides a function to set string type to cell.
509512
func trimCellValue(value string, escape bool) (v string, ns xml.Attr) {
510-
value = truncateUTF16Units(value, TotalCellChars)
513+
if len(utf16.Encode([]rune(value))) > TotalCellChars {
514+
value = truncateUTF16Units(value, TotalCellChars)
515+
}
511516
if value != "" {
512517
prefix, suffix := value[0], value[len(value)-1]
513518
for _, ascii := range []byte{9, 10, 13, 32} {
@@ -1206,7 +1211,7 @@ func setRichText(runs []RichTextRun) ([]xlsxR, error) {
12061211
totalCellChars int
12071212
)
12081213
for _, textRun := range runs {
1209-
totalCellChars += utf16UnitCountInString(textRun.Text)
1214+
totalCellChars += len(utf16.Encode([]rune(textRun.Text)))
12101215
if totalCellChars > TotalCellChars {
12111216
return textRuns, ErrCellCharsLength
12121217
}

‎cell_test.go

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -245,34 +245,6 @@ func TestSetCellValuesMultiByte(t *testing.T) {
245245
assert.NoError(t, f.SaveAs(filepath.Join("test", "TestSetCellValuesMultiByte.xlsx")))
246246
}
247247

248-
func TestSetCellValuesSurrogatePairString(t *testing.T) {
249-
f := NewFile()
250-
row := []interface{}{
251-
// Test set cell value with surrogate pair characters value
252-
strings.Repeat("😀", TotalCellChars),
253-
strings.Repeat("A", TotalCellChars-1) + "😀",
254-
}
255-
assert.NoError(t, f.SetSheetRow("Sheet1", "A1", &row))
256-
// Test set cell value with XML escape characters in stream writer
257-
_, err := f.NewSheet("Sheet2")
258-
assert.NoError(t, err)
259-
streamWriter, err := f.NewStreamWriter("Sheet2")
260-
assert.NoError(t, err)
261-
assert.NoError(t, streamWriter.SetRow("A1", row))
262-
assert.NoError(t, streamWriter.Flush())
263-
for _, sheetName := range []string{"Sheet1", "Sheet2"} {
264-
for cell, expected := range map[string]int{
265-
"A1": TotalCellChars / 2,
266-
"B1": TotalCellChars - 1,
267-
} {
268-
result, err := f.GetCellValue(sheetName, cell)
269-
assert.NoError(t, err)
270-
assert.Len(t, []rune(result), expected)
271-
}
272-
}
273-
assert.NoError(t, f.SaveAs(filepath.Join("test", "TestSetCellValuesSurrogatePairString.xlsx")))
274-
}
275-
276248
func TestSetCellValue(t *testing.T) {
277249
f := NewFile()
278250
assert.Equal(t, newCellNameToCoordinatesError("A", newInvalidCellNameError("A")), f.SetCellValue("Sheet1", "A", time.Now().UTC()))

‎lib.go

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -941,22 +941,12 @@ func setPtrFieldsVal(fields []string, immutable, mutable reflect.Value) {
941941
}
942942
}
943943

944-
// utf16UnitCountInString returns the number of UTF-16 code units in a string.
945-
func utf16UnitCountInString(s string) int {
946-
var count int
947-
for _, r := range s {
948-
count += utf16.RuneLen(r)
949-
}
950-
return count
951-
}
952-
953-
// truncateUTF16Units truncates a string to a maximum number of UTF-16 code units.
954-
func truncateUTF16Units(s string, maxLength int) string {
955-
var count int
944+
// truncateUTF16Units truncates a string to a maximum number of UTF-16 code
945+
// units.
946+
func truncateUTF16Units(s string, length int) string {
947+
var cnt int
956948
for i, r := range s {
957-
count += utf16.RuneLen(r)
958-
if count > maxLength {
959-
// If s[maxLength-1] is a high surrogate, it is also truncated.
949+
if cnt += utf16.RuneLen(r); cnt > length {
960950
return s[:i]
961951
}
962952
}

‎lib_test.go

Lines changed: 23 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"strings"
1212
"sync"
1313
"testing"
14+
"unicode/utf16"
1415

1516
"github.com/stretchr/testify/assert"
1617
"github.com/stretchr/testify/require"
@@ -353,6 +354,28 @@ func TestBstrMarshal(t *testing.T) {
353354
}
354355
}
355356

357+
func TestTruncateUTF16Units(t *testing.T) {
358+
assertTrunc := func(s string, max int, expected string) {
359+
assert.Equal(t, expected, truncateUTF16Units(s, max), "src=%q max=%d", s, max)
360+
assert.LessOrEqual(t, len(utf16.Encode([]rune(truncateUTF16Units(s, max)))), max)
361+
}
362+
// No truncation
363+
assertTrunc("ABC", 3, "ABC")
364+
assertTrunc("A\U0001F600B", 4, "A\U0001F600B")
365+
// Truncate cutting before BMP rune
366+
assertTrunc("ABCDE", 3, "ABC")
367+
// Truncate with surrogate pair boundary: keep pair intact
368+
assertTrunc("A\U0001F600B", 3, "A\U0001F600") // 1 + 2 units
369+
assertTrunc("A\U0001F600B", 2, "A") // pair would overflow
370+
assertTrunc("\U0001F600B", 1, "") // first rune (2 units) exceeds limit
371+
assertTrunc("\U0001F600B", 2, "\U0001F600") // exact fit
372+
assertTrunc("\U0001F600B", 3, "\U0001F600B") // allow extra
373+
// Multiple surrogate pairs
374+
assertTrunc("\U0001F600\U0001F600B", 2, "\U0001F600") // corrected expectation per logic
375+
assertTrunc("\U0001F600\U0001F600B", 3, "\U0001F600") // 2 units kept, next pair would exceed
376+
assertTrunc("\U0001F600\U0001F600B", 4, "\U0001F600\U0001F600") // both pairs (4 units)
377+
}
378+
356379
func TestReadBytes(t *testing.T) {
357380
f := &File{tempFiles: sync.Map{}}
358381
sheet := "xl/worksheets/sheet1.xml"
@@ -390,45 +413,3 @@ func TestUnzipToTemp(t *testing.T) {
390413
_, err = f.unzipToTemp(z.File[0])
391414
assert.EqualError(t, err, "EOF")
392415
}
393-
394-
func TestUTF16UnitCountInString(t *testing.T) {
395-
cases := map[string]int{
396-
"": 0,
397-
"ABC": 3, // all Basic Multilingual Plane 1 unit each
398-
"你好": 2, // Chinese Basic Multilingual Plane chars
399-
"😀": 2, // single surrogate pair
400-
"A😀B": 4, // 1 + 2 + 1
401-
"😀𩸽": 4, // two surrogate pairs
402-
}
403-
for s, expected := range cases {
404-
assert.Equal(t, expected, utf16UnitCountInString(s), s)
405-
}
406-
}
407-
408-
func TestTruncateUTF16Units(t *testing.T) {
409-
// helper to assert
410-
assertTrunc := func(s string, max int, expected string) {
411-
assert.Equalf(t, expected, truncateUTF16Units(s, max), "src=%q max=%d", s, max)
412-
// ensure result doesn't exceed max units
413-
assert.LessOrEqual(t, utf16UnitCountInString(truncateUTF16Units(s, max)), max)
414-
}
415-
416-
// No truncation
417-
assertTrunc("ABC", 3, "ABC")
418-
assertTrunc("A😀B", 4, "A😀B")
419-
420-
// Truncate cutting before BMP rune
421-
assertTrunc("ABCDE", 3, "ABC")
422-
423-
// Truncate with surrogate pair boundary: keep pair intact
424-
assertTrunc("A😀B", 3, "A😀") // 1 + 2 units
425-
assertTrunc("A😀B", 2, "A") // pair would overflow
426-
assertTrunc("😀B", 1, "") // first rune (2 units) exceeds limit
427-
assertTrunc("😀B", 2, "😀") // exact fit
428-
assertTrunc("😀B", 3, "😀B") // allow extra
429-
430-
// Multiple surrogate pairs
431-
assertTrunc("😀😀B", 2, "😀") // corrected expectation per logic
432-
assertTrunc("😀😀B", 3, "😀") // 2 units kept, next pair would exceed
433-
assertTrunc("😀😀B", 4, "😀😀") // both pairs (4 units)
434-
}

‎sheet.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1484,7 +1484,7 @@ func checkSheetName(name string) error {
14841484
if name == "" {
14851485
return ErrSheetNameBlank
14861486
}
1487-
if utf16UnitCountInString(name) > MaxSheetNameLength {
1487+
if len(utf16.Encode([]rune(name))) > MaxSheetNameLength {
14881488
return ErrSheetNameLength
14891489
}
14901490
if strings.HasPrefix(name, "'") || strings.HasSuffix(name, "'") {

‎sheet_test.go

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -762,26 +762,28 @@ func TestSetSheetBackgroundFromBytes(t *testing.T) {
762762
}
763763

764764
func TestCheckSheetName(t *testing.T) {
765-
// Test valid sheet name
766-
assert.NoError(t, checkSheetName("Sheet1"))
767-
assert.NoError(t, checkSheetName("She'et1"))
768-
assert.NoError(t, checkSheetName("Sheet😀😀😀😀😀😀😀😀😀😀😀😀😀")) // 5+13*2=31 UTF-16 units
769-
// Test invalid sheet name, empty name
770-
assert.EqualError(t, checkSheetName(""), ErrSheetNameBlank.Error())
771-
// Test invalid sheet name, include :\/?*[]
772-
assert.EqualError(t, checkSheetName("Sheet:"), ErrSheetNameInvalid.Error())
773-
assert.EqualError(t, checkSheetName(`Sheet\`), ErrSheetNameInvalid.Error())
774-
assert.EqualError(t, checkSheetName("Sheet/"), ErrSheetNameInvalid.Error())
775-
assert.EqualError(t, checkSheetName("Sheet?"), ErrSheetNameInvalid.Error())
776-
assert.EqualError(t, checkSheetName("Sheet*"), ErrSheetNameInvalid.Error())
777-
assert.EqualError(t, checkSheetName("Sheet["), ErrSheetNameInvalid.Error())
778-
assert.EqualError(t, checkSheetName("Sheet]"), ErrSheetNameInvalid.Error())
779-
// Test invalid sheet name, single quotes at the front or at the end
780-
assert.EqualError(t, checkSheetName("'Sheet"), ErrSheetNameSingleQuote.Error())
781-
assert.EqualError(t, checkSheetName("Sheet'"), ErrSheetNameSingleQuote.Error())
782-
// Test invalid sheet name, exceed max length
783-
assert.EqualError(t, checkSheetName("Sheet678901234567890123456789012"), ErrSheetNameLength.Error())
784-
assert.EqualError(t, checkSheetName("Sheet😀😀😀😀😀😀😀😀😀😀😀😀😀😀"), ErrSheetNameLength.Error()) // 5+14*2=33 UTF-16 units
765+
for expected, name := range map[error]string{
766+
// Test valid sheet name
767+
nil: "Sheet1",
768+
nil: "She'et1",
769+
// Test invalid sheet name, empty name
770+
ErrSheetNameBlank: "",
771+
// Test invalid sheet name, include :\/?*[]
772+
ErrSheetNameInvalid: "Sheet:",
773+
ErrSheetNameInvalid: `Sheet\`,
774+
ErrSheetNameInvalid: "Sheet/",
775+
ErrSheetNameInvalid: "Sheet?",
776+
ErrSheetNameInvalid: "Sheet*",
777+
ErrSheetNameInvalid: "Sheet[",
778+
ErrSheetNameInvalid: "Sheet]",
779+
// Test invalid sheet name, single quotes at the front or at the end
780+
ErrSheetNameSingleQuote: "'Sheet",
781+
ErrSheetNameSingleQuote: "Sheet'",
782+
// Test invalid sheet name, exceed max length
783+
ErrSheetNameLength: "Sheet" + strings.Repeat("\U0001F600", 14),
784+
} {
785+
assert.Equal(t, expected, checkSheetName(name))
786+
}
785787
}
786788

787789
func TestSheetDimension(t *testing.T) {

0 commit comments

Comments
 (0)