Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix simpleCellAddressFromString crashes when called with non-ASCII character on an unquoted sheet name #1314

Merged
merged 6 commits into from
Oct 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Fixed

- Fixed an issue where operation on ranges with incompatible sizes resulted in a runtime exception. [#1267](https://github.com/handsontable/hyperformula/issues/1267)
- Fixed an issue where `simpleCellAddressFromString` method was crashing when called with a non-ASCII character in an unquoted sheet name. [#1312](https://github.com/handsontable/hyperformula/issues/1312)

## [2.6.0] - 2023-09-19

Expand Down
17 changes: 9 additions & 8 deletions src/HyperFormula.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2751,10 +2751,11 @@ export class HyperFormula implements TypedEmitter {

/**
* Computes simple (absolute) address of a cell address based on its string representation.
* If sheet name is present in string representation but not present in the engine, returns `undefined`.
* - If sheet name is present in the string representation but is not present in the engine, returns `undefined`.
* - If sheet name is not present in the string representation, returns `contextSheetId` as sheet number.
*
* @param {string} cellAddress - string representation of cell address in A1 notation
* @param {number} sheetId - context used in case of missing sheet in the first argument
* @param {number} contextSheetId - context used in case of missing sheet in the first argument
*
* @throws [[ExpectedValueOfTypeError]] if any of its basic type argument is of wrong type
*
Expand All @@ -2767,21 +2768,21 @@ export class HyperFormula implements TypedEmitter {
* const simpleCellAddress = hfInstance.simpleCellAddressFromString('A1', 0);
*
* // returns { sheet: 0, col: 0, row: 5 }
* const simpleCellAddressTwo = hfInstance.simpleCellAddressFromString('Sheet1!A6');
* const simpleCellAddress = hfInstance.simpleCellAddressFromString('Sheet1!A6');
*
* // returns { sheet: 0, col: 0, row: 5 }
* const simpleCellAddressTwo = hfInstance.simpleCellAddressFromString('Sheet1!$A$6');
* const simpleCellAddress = hfInstance.simpleCellAddressFromString('Sheet1!$A$6');
*
* // returns 'undefined', as there's no 'Sheet 2' in the HyperFormula instance
* const simpleCellAddressTwo = hfInstance.simpleCellAddressFromString('Sheet2!A6');
* const simpleCellAddress = hfInstance.simpleCellAddressFromString('Sheet2!A6');
* ```
*
* @category Helpers
*/
public simpleCellAddressFromString(cellAddress: string, sheetId: number): SimpleCellAddress | undefined {
public simpleCellAddressFromString(cellAddress: string, contextSheetId: number): SimpleCellAddress | undefined {
validateArgToType(cellAddress, 'string', 'cellAddress')
validateArgToType(sheetId, 'number', 'sheetId')
return simpleCellAddressFromString(this.sheetMapping.get, cellAddress, sheetId)
validateArgToType(contextSheetId, 'number', 'sheetId')
return simpleCellAddressFromString(this.sheetMapping.get, cellAddress, contextSheetId)
}

/**
Expand Down
26 changes: 15 additions & 11 deletions src/parser/addressRepresentationConverters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,39 +97,43 @@ export const rowAddressFromString = (sheetMapping: SheetMappingFn, stringAddress

/**
* Computes simple (absolute) address of a cell address based on its string representation.
* If sheet name present in string representation but is not present in sheet mapping, returns undefined.
* If sheet name is not present in string representation, returns {@param sheetContext} as sheet number
* - If sheet name is present in the string representation but is not present in sheet mapping, returns `undefined`.
* - If sheet name is not present in the string representation, returns {@param contextSheetId} as sheet number.
*
* @param sheetMapping - mapping function needed to change name of a sheet to index
* @param stringAddress - string representation of cell address, e.g. 'C64'
* @param sheetContext - sheet in context of which we should parse the address
* @param contextSheetId - sheet in context of which we should parse the address
* @returns absolute representation of address, e.g. { sheet: 0, col: 1, row: 1 }
*/
export const simpleCellAddressFromString = (sheetMapping: SheetMappingFn, stringAddress: string, sheetContext: number): Maybe<SimpleCellAddress> => {
const result = addressRegex.exec(stringAddress)!
export const simpleCellAddressFromString = (sheetMapping: SheetMappingFn, stringAddress: string, contextSheetId: number): Maybe<SimpleCellAddress> => {
const regExpExecArray = addressRegex.exec(stringAddress)!

const col = columnLabelToIndex(result[6])
if (!regExpExecArray) {
return undefined
}

let sheet = extractSheetNumber(result, sheetMapping)
const col = columnLabelToIndex(regExpExecArray[6])

let sheet = extractSheetNumber(regExpExecArray, sheetMapping)
if (sheet === undefined) {
return undefined
}

if (sheet === null) {
sheet = sheetContext
sheet = contextSheetId
}

const row = Number(result[8]) - 1
const row = Number(regExpExecArray[8]) - 1
return simpleCellAddress(sheet, col, row)
}

export const simpleCellRangeFromString = (sheetMapping: SheetMappingFn, stringAddress: string, sheetContext: number): Maybe<SimpleCellRange> => {
export const simpleCellRangeFromString = (sheetMapping: SheetMappingFn, stringAddress: string, contextSheetId: number): Maybe<SimpleCellRange> => {
const split = stringAddress.split(RANGE_OPERATOR)
if (split.length !== 2) {
return undefined
}
const [startString, endString] = split
const start = simpleCellAddressFromString(sheetMapping, startString, sheetContext)
const start = simpleCellAddressFromString(sheetMapping, startString, contextSheetId)
if (start === undefined) {
return undefined
}
Expand Down
10 changes: 5 additions & 5 deletions test/address-representation-converters.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,24 @@ describe('simpleCellAddressFromString', () => {
return index > 0 ? index : undefined
}

it('should return simple cell address', () => {
it('should return a simple cell address when called with a valid string address', () => {
expect(simpleCellAddressFromString(sheetMappingFunction, 'A1', 0)).toEqual(adr('A1'))
expect(simpleCellAddressFromString(sheetMappingFunction, 'AY7', 0)).toEqual(simpleCellAddress(0, 50, 6))
})

it('should return undefined', () => {
it('should return undefined when sheet does not exist', () => {
expect(simpleCellAddressFromString(sheetMappingFunction, 'Sheet4!A1', 0)).toBeUndefined()
})

it('should return address with overridden sheet', () => {
it('should return address with context sheet when string address does not contain a sheet name', () => {
expect(simpleCellAddressFromString(sheetMappingFunction, 'A1', 1)).toEqual(adr('A1', 1))
})

it('should return address with sheet number from sheet mapping', () => {
it('should return a valid address when called with a valid sheet name', () => {
expect(simpleCellAddressFromString(sheetMappingFunction, 'Sheet2!A1', 1)).toEqual(adr('A1', 1))
})

it('should return address with sheet number from sheet mapping regardless of override parameter', () => {
it('should return address with sheet number from sheet mapping regardless of context sheet parameter', () => {
expect(simpleCellAddressFromString(sheetMappingFunction, 'Sheet3!A1', 1)).toEqual(adr('A1', 2))
})
})
Expand Down
26 changes: 24 additions & 2 deletions test/engine.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import {DetailedCellError, ErrorType, HyperFormula, CellType, CellValueDetailedType, CellValueType, SimpleCellAddress, SimpleCellRange} from '../src'
import {DetailedCellError, ErrorType, HyperFormula, CellType, CellValueDetailedType, CellValueType, SimpleCellAddress, SimpleCellRange, ExpectedValueOfTypeError} from '../src'
import {AbsoluteCellRange, simpleCellRange} from '../src/AbsoluteCellRange'
import {Config} from '../src/Config'
import {ErrorMessage} from '../src/error-message'
import {plPL} from '../src/i18n/languages'
import {adr, detailedError, expectArrayWithSameContent} from './testUtils'
import {ExpectedValueOfTypeError} from '../src/errors'
import {simpleCellAddress} from '../src/Cell'

describe('#buildFromArray', () => {
Expand Down Expand Up @@ -1069,6 +1068,29 @@ describe('#simpleCellAddressFromString', () => {
const engine = HyperFormula.buildEmpty()
expect(engine.simpleCellAddressFromString('C5', 2)).toEqual(simpleCellAddress(2, 2, 4))
})

it('should work for address with sheet name', () => {
const engine = HyperFormula.buildFromSheets({
Sheet1: [],
})
expect(engine.simpleCellAddressFromString('Sheet1!C5', 2)).toEqual(simpleCellAddress(0, 2, 4))
})

it('should work when sheet name contains unicode characters (sheet name in single quotes)', () => {
// noinspection NonAsciiCharacters
const engine = HyperFormula.buildFromSheets({
あは: [],
})
expect(engine.simpleCellAddressFromString("'あは'!C5", 2)).toEqual(simpleCellAddress(0, 2, 4))
})

it('should return undefined when sheet name contains unicode characters (unquoted sheet name)', () => {
// noinspection NonAsciiCharacters
const engine = HyperFormula.buildFromSheets({
あは: [],
})
expect(engine.simpleCellAddressFromString('あは!C5', 2)).toBe(undefined)
})
})

describe('#simpleCellRangeFromString', () => {
Expand Down
Loading