Skip to content

fix:non-string values #616

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.

Sign up for GitHub

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

Merged
carlbrugger merged 1 commit into from
Sep 23, 2024
Merged

fix:non-string values #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions .changeset/wise-zoos-smoke.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@flatfile/plugin-xlsx-extractor': patch
'@flatfile/util-extractor': patch
---

This release fixes an issue when using `raw` or `rawNumbers` with non-string values in the header row.
3 changes: 2 additions & 1 deletion plugins/xlsx-extractor/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ export function prependNonUniqueHeaderColumns(

for (const [key, value] of Object.entries(record)) {
const newValue = value ? value : 'empty'
Comment on lines 7 to 8

@coderabbitai coderabbitai Bot Sep 23, 2024

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.

Consider revising empty value handling

The current implementation converts empty values to the string 'empty'. This approach might not be ideal in all scenarios, especially if distinguishing between truly empty values and the string 'empty' is important.

Consider allowing empty values to remain as empty strings or use a more distinct placeholder. For example:

- const newValue = value ? value : 'empty'
+ const newValue = value ?? ''  // or use a symbol like Symbol('empty')

This change would preserve the emptiness of the original value while still allowing for duplicate detection.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (const [key, value] of Object.entries(record)) {
const newValue = value ? value : 'empty'
for (const [key, value] of Object.entries(record)) {
const newValue = value ?? '' // or use a symbol like Symbol('empty')

const cleanValue = newValue.replace('*', '')
const cleanValue =
typeof newValue === 'string' ? newValue.replace('*', '') : newValue

if (cleanValue && counts[cleanValue]) {
result[key] = `${cleanValue}_${counts[cleanValue]}`
Expand Down
3 changes: 3 additions & 0 deletions utils/extractor/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,9 @@ export function keysToFields({
{ count: number; index: number; metadata?: { fieldRef: string } }
> = keys.reduce((acc, key) => {
if (!key) key = ''
if (typeof key !== 'string') {
key = String(key)
}
Comment on lines +240 to +242

@coderabbitai coderabbitai Bot Sep 23, 2024

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.

Approve changes with a minor suggestion for improvement

The added type check and string conversion effectively address the issue of non-string values in the header row, which aligns with the PR objectives. This change should resolve the XLSX file extraction failures mentioned in the linked issue #1583.

Consider adding a more robust type conversion to handle potential edge cases:

 if (typeof key !== 'string') {
-  key = String(key)
+  key = typeof key === 'object' && key !== null ? JSON.stringify(key) : String(key)
 }

This modification ensures that complex objects are properly stringified, providing more meaningful header values in such cases.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (typeof key !== 'string') {
key = String(key)
}
if (typeof key !== 'string') {
key = typeof key === 'object' && key !== null ? JSON.stringify(key) : String(key)
}

Tip

Codebase Verification

Potential Issues Found with Non-String Key Processing

The verification revealed multiple instances where keys are processed without explicit type checking or conversion. It's recommended to review these areas to ensure consistent and robust handling of non-string keys:

  • utils/extractor/src/index.ts
    • key.trim().replace(/%/g, '_PERCENT_').replace(/\$/g, '_DOLLAR_')
    • key = key.trim()
  • utils/common/src/simple.records.ts
    • Multiple uses of Object.entries(obj) and related methods
  • plugins/xlsx-extractor/src/parser.ts
    • Various Object.keys(workbook.Sheets) and similar patterns
  • plugins/record-hook/src/record.translator.ts
    • Uses of Object.entries(record.values) and related methods
  • ...and several other files as identified in the shell script output.

Ensure that all key manipulations include appropriate type checks or conversions to handle non-string values safely.

Analysis chain

Suggest comprehensive testing

The changes look good and address the issue effectively. To ensure robustness, it would be beneficial to conduct comprehensive testing with various types of non-string header values (e.g., dates, numbers, booleans, and potentially nested objects).

To assist with verification, you can use the following script to check for any remaining instances where non-string keys might be processed without conversion:

This script will help identify areas in the codebase where keys are being processed, potentially without proper type checking. Review the results to ensure that appropriate safeguards are in place throughout the project.

Update Verification Script to Correctly Target TypeScript Files

The previous verification script encountered an issue recognizing the typescript file type. To ensure accurate searching, please use the following updated script that targets .ts and .tsx files explicitly:

This script explicitly searches for .ts and .tsx files, ensuring that TypeScript files are correctly targeted. Please execute this updated script and review the results to verify that all potential non-string key processing instances are appropriately handled.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential non-string key processing in the codebase

# Test 1: Search for direct key access or manipulation without type checking
echo "Test 1: Searching for potential unchecked key access..."
rg --type typescript --type javascript 'key\s*\.\s*(trim|toLowerCase|toUpperCase|replace|substring|slice)' -g '!*test*'

# Test 2: Search for array or object methods that might assume string keys
echo "Test 2: Searching for methods that might assume string keys..."
rg --type typescript --type javascript '(Object\.keys|Object\.entries|Object\.values|for\s*\(\s*\w+\s+in\s)' -g '!*test*'

echo "Review the results to ensure proper type checking is in place where necessary."

Length of output: 738

Script:

#!/bin/bash
# Description: Check for potential non-string key processing in the codebase

# Test 1: Search for direct key access or manipulation without type checking
echo "Test 1: Searching for potential unchecked key access..."
rg --glob '*.ts' --glob '*.tsx' 'key\s*\.\s*(trim|toLowerCase|toUpperCase|replace|substring|slice)' -g '!*test*'

# Test 2: Search for array or object methods that might assume string keys
echo "Test 2: Searching for methods that might assume string keys..."
rg --glob '*.ts' --glob '*.tsx' '(Object\.keys|Object\.entries|Object\.values|for\s*\(\s*\w+\s+in\s)' -g '!*test*'

echo "Review the results to ensure proper type checking is in place where necessary."

Length of output: 4657

key = key.trim()
if (key === '') {
key = 'empty'
Expand Down