Skip to content
New issue

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

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

Already on GitHub? # to your account

Update recordid.ts #393

Merged
merged 6 commits into from
Feb 1, 2025
Merged

Update recordid.ts #393

merged 6 commits into from
Feb 1, 2025

Conversation

CLoaKY233
Copy link
Contributor

@CLoaKY233 CLoaKY233 commented Feb 1, 2025

Fix: Proper Escaping of Long Numeric String Record IDs

Issue Description

ISSUE ---> #389 (comment)
When handling RecordId instances with long numeric strings as identifiers (15 or more digits), the current implementation doesn't properly escape these values. This causes inconsistency with SurrealDB's native behavior, which wraps such identifiers in angle brackets (⟨⟩).

Current Behavior

new RecordId('foo', '125813199042576601589342522460260755').toString()
// Returns: foo:125813199042576601589342522460260755

Expected Behavior

new RecordId('foo', '125813199042576601589342522460260755').toString()
// Should return: foo:⟨125813199042576601589342522460260755⟩

Changes Made

  1. Modified escapeIdPart function to detect and properly escape long numeric string IDs
  2. Updated StringRecordId constructor to handle long numeric string IDs consistently
  3. Added specific handling for IDs matching the pattern /^\d{15,}$/

Technical Details

  • Added check for numeric strings of 15 or more digits
  • Implemented wrapping with angle brackets (⟨⟩) for matching IDs
  • Preserved existing behavior for all other ID types
  • Maintains backward compatibility with existing implementations

Testing

Tested with various ID types including:

  • Regular string IDs
  • Short numeric strings
  • Long numeric strings
  • UUIDs
  • Complex objects and arrays

Examples

// Long numeric string ID
new RecordId('table', '125813199042576601589342522460260755')
// => table:⟨125813199042576601589342522460260755⟩

// Regular string ID (unchanged behavior)
new RecordId('table', 'regular-id')
// => table:regular-id

// Numeric ID (unchanged behavior)
new RecordId('table', 123)
// => table:123

@CLoaKY233 CLoaKY233 requested a review from kearfy as a code owner February 1, 2025 19:27
@kearfy
Copy link
Member

kearfy commented Feb 1, 2025

Thank you for your PR @CLoaKY233! Left two comments 🙏

@CLoaKY233
Copy link
Contributor Author

Following the review feedback, I've simplified the fix to focus on the root cause:

  1. Changed isOnlyNumbers function to use regex instead of parseInt:
function isOnlyNumbers(str: string): boolean {
    return /^\d+$/.test(str.replace(/_/g, ''));
}

Key improvements:

  • Replaced parseInt-based check with regex pattern /^\d+$/
  • Added global flag to underscore replacement (/_/g) to handle multiple underscores
  • Removed unnecessary string conversion and comparison
  • More reliable detection of numeric strings of any length

@kearfy
Copy link
Member

kearfy commented Feb 1, 2025

Awesome @CLoaKY233, thank you so much! One more favor, could you run bun quality:apply:unsafe to fix the "Code Quality" test? Thank you 🙏

@CLoaKY233
Copy link
Contributor Author

Done Sir! :D

@kearfy kearfy merged commit 4556798 into surrealdb:main Feb 1, 2025
12 checks passed
@kearfy
Copy link
Member

kearfy commented Feb 1, 2025

Thanks for bearing with me @CLoaKY233! Merged it!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants