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.

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

Fix: sort document reference by long type id #2257

Merged
merged 7 commits into from
Dec 17, 2024
Merged

Fix: sort document reference by long type id #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
19300ef
change the logic for documentId comparison
milaGGL Dec 6, 2024
679cba8
use Math.sign to compare numeric values
milaGGL Dec 6, 2024
4b6c02c
add largest and smallest long number to the test
milaGGL Dec 10, 2024
afa527c
remove math.sign comparision on numbers
milaGGL Dec 12, 2024
81e70a4
Update path.ts
milaGGL Dec 12, 2024
e49959d
resolve comment
milaGGL Dec 13, 2024
a338b77
add "__id" to the tests
milaGGL Dec 16, 2024
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
67 changes: 59 additions & 8 deletions dev/src/path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ abstract class Path<T> {
/**
* Compare the current path against another Path object.
*
* Compare the current path against another Path object. Paths are compared segment by segment,
* prioritizing numeric IDs (e.g., "__id123__") in numeric ascending order, followed by string
* segments in lexicographical order.
*
* @private
* @internal
* @param other The path to compare to.
Expand All @@ -158,20 +162,67 @@ abstract class Path<T> {
compareTo(other: Path<T>): number {
const len = Math.min(this.segments.length, other.segments.length);
for (let i = 0; i < len; i++) {
if (this.segments[i] < other.segments[i]) {
return -1;
}
if (this.segments[i] > other.segments[i]) {
return 1;
const comparison = this.compareSegments(
this.segments[i],
other.segments[i]
);
if (comparison !== 0) {
return comparison;
}
}
if (this.segments.length < other.segments.length) {
return Math.sign(this.segments.length - other.segments.length);
}

private compareSegments(lhs: string, rhs: string): number {
const isLhsNumeric = this.isNumericId(lhs);
const isRhsNumeric = this.isNumericId(rhs);

if (isLhsNumeric && !isRhsNumeric) {
// Only lhs is numeric
return -1;
} else if (!isLhsNumeric && isRhsNumeric) {
// Only rhs is numeric
return 1;
} else if (isLhsNumeric && isRhsNumeric) {
// both numeric
return this.compareNumbers(
this.extractNumericId(lhs),
this.extractNumericId(rhs)
);
} else {
// both non-numeric
return this.compareStrings(lhs, rhs);
}
}

// Checks if a segment is a numeric ID (starts with "__id" and ends with "__").
private isNumericId(segment: string): boolean {
return segment.startsWith('__id') && segment.endsWith('__');
}

// Extracts the long number from a numeric ID segment.
private extractNumericId(segment: string): bigint {
return BigInt(segment.substring(4, segment.length - 2));
}

private compareNumbers(lhs: bigint, rhs: bigint): number {
if (lhs < rhs) {
return -1;
} else if (lhs > rhs) {
return 1;
} else {
return 0;
}
if (this.segments.length > other.segments.length) {
}

private compareStrings(lhs: string, rhs: string): number {
if (lhs < rhs) {
return -1;
} else if (lhs > rhs) {
return 1;
} else {
return 0;
}
return 0;
}

/**
Expand Down
106 changes: 106 additions & 0 deletions dev/system-test/firestore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3913,6 +3913,112 @@ describe('Query class', () => {
unsubscribe();
});

it('snapshot listener sorts query by DocumentId same way as server', async () => {
milaGGL marked this conversation as resolved.
Show resolved Hide resolved
const batch = firestore.batch();
batch.set(randomCol.doc('A'), {a: 1});
batch.set(randomCol.doc('a'), {a: 1});
batch.set(randomCol.doc('Aa'), {a: 1});
batch.set(randomCol.doc('7'), {a: 1});
batch.set(randomCol.doc('12'), {a: 1});
batch.set(randomCol.doc('__id7__'), {a: 1});
batch.set(randomCol.doc('__id12__'), {a: 1});
batch.set(randomCol.doc('__id-2__'), {a: 1});
batch.set(randomCol.doc('__id1_'), {a: 1});
batch.set(randomCol.doc('_id1__'), {a: 1});
batch.set(randomCol.doc('__id'), {a: 1});
// largest long number
batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1});
batch.set(randomCol.doc('__id9223372036854775806__'), {a: 1});
// smallest long number
batch.set(randomCol.doc('__id-9223372036854775808__'), {a: 1});
batch.set(randomCol.doc('__id-9223372036854775807__'), {a: 1});
await batch.commit();

const query = randomCol.orderBy(FieldPath.documentId());
const expectedDocs = [
'__id-9223372036854775808__',
'__id-9223372036854775807__',
'__id-2__',
'__id7__',
'__id12__',
'__id9223372036854775806__',
'__id9223372036854775807__',
'12',
'7',
'A',
'Aa',
'__id',
'__id1_',
'_id1__',
'a',
];

const getSnapshot = await query.get();
expect(getSnapshot.docs.map(d => d.id)).to.deep.equal(expectedDocs);

const unsubscribe = query.onSnapshot(snapshot =>
currentDeferred.resolve(snapshot)
);

const watchSnapshot = await waitForSnapshot();
// Compare the snapshot (including sort order) of a snapshot
snapshotsEqual(watchSnapshot, {
docs: getSnapshot.docs,
docChanges: getSnapshot.docChanges(),
});
unsubscribe();
});

it('snapshot listener sorts filtered query by DocumentId same way as server', async () => {
const batch = firestore.batch();
batch.set(randomCol.doc('A'), {a: 1});
batch.set(randomCol.doc('a'), {a: 1});
batch.set(randomCol.doc('Aa'), {a: 1});
batch.set(randomCol.doc('7'), {a: 1});
batch.set(randomCol.doc('12'), {a: 1});
batch.set(randomCol.doc('__id7__'), {a: 1});
batch.set(randomCol.doc('__id12__'), {a: 1});
batch.set(randomCol.doc('__id-2__'), {a: 1});
batch.set(randomCol.doc('__id1_'), {a: 1});
batch.set(randomCol.doc('_id1__'), {a: 1});
batch.set(randomCol.doc('__id'), {a: 1});
// largest long number
batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1});
batch.set(randomCol.doc('__id9223372036854775806__'), {a: 1});
// smallest long number
batch.set(randomCol.doc('__id-9223372036854775808__'), {a: 1});
batch.set(randomCol.doc('__id-9223372036854775807__'), {a: 1});
await batch.commit();

const query = randomCol
.where(FieldPath.documentId(), '>', '__id7__')
.where(FieldPath.documentId(), '<=', 'A')
.orderBy(FieldPath.documentId());
const expectedDocs = [
'__id12__',
'__id9223372036854775806__',
'__id9223372036854775807__',
'12',
'7',
'A',
];

const getSnapshot = await query.get();
expect(getSnapshot.docs.map(d => d.id)).to.deep.equal(expectedDocs);

const unsubscribe = query.onSnapshot(snapshot =>
currentDeferred.resolve(snapshot)
);

const watchSnapshot = await waitForSnapshot();
// Compare the snapshot (including sort order) of a snapshot
snapshotsEqual(watchSnapshot, {
docs: getSnapshot.docs,
docChanges: getSnapshot.docChanges(),
});
unsubscribe();
});

it('SDK orders vector field same way as backend', async () => {
// We validate that the SDK orders the vector field the same way as the backend
// by comparing the sort order of vector fields from a Query.get() and
Expand Down
Loading