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 #14248

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix: sort document reference by long type id #14248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d798016
sort document keys by numeric ids
milaGGL Dec 12, 2024
cb91283
Update base_path.h
milaGGL Dec 12, 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
89 changes: 89 additions & 0 deletions Firestore/Example/Tests/Integration/API/FIRQueryTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,95 @@ - (void)testCollectionGroupQueriesWithStartAtEndAtWithArbitraryDocumentIDs {
XCTAssertEqualObjects(ids, (@[ @"cg-doc2" ]));
}

- (void)testSnapshotListenerSortsQueryByDocumentIdInTheSameOrderAsServer {
FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
@"A" : @{@"a" : @1},
@"a" : @{@"a" : @1},
@"Aa" : @{@"a" : @1},
@"7" : @{@"a" : @1},
@"12" : @{@"a" : @1},
@"__id7__" : @{@"a" : @1},
@"__id12__" : @{@"a" : @1},
@"__id-2__" : @{@"a" : @1},
@"__id1_" : @{@"a" : @1},
@"_id1__" : @{@"a" : @1},
@"__id9223372036854775807__" : @{@"a" : @1},
@"__id-9223372036854775808__" : @{@"a" : @1},
}];

FIRQuery *query = [collRef queryOrderedByFieldPath:[FIRFieldPath documentID]];
NSArray<NSString *> *expectedDocs = @[
@"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__",
@"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id1_", @"_id1__", @"a"
];
FIRQuerySnapshot *getSnapshot = [self readDocumentSetForRef:query];
XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(getSnapshot), expectedDocs);

id<FIRListenerRegistration> registration =
[query addSnapshotListener:self.eventAccumulator.valueEventHandler];
FIRQuerySnapshot *watchSnapshot = [self.eventAccumulator awaitEventWithName:@"Snapshot"];
XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(watchSnapshot), expectedDocs);

[registration remove];
}

- (void)testSnapshotListenerSortsFilteredQueryByDocumentIdInTheSameOrderAsServer {
FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
@"A" : @{@"a" : @1},
@"a" : @{@"a" : @1},
@"Aa" : @{@"a" : @1},
@"7" : @{@"a" : @1},
@"12" : @{@"a" : @1},
@"__id7__" : @{@"a" : @1},
@"__id12__" : @{@"a" : @1},
@"__id-2__" : @{@"a" : @1},
@"__id1_" : @{@"a" : @1},
@"_id1__" : @{@"a" : @1},
@"__id9223372036854775807__" : @{@"a" : @1},
@"__id-9223372036854775808__" : @{@"a" : @1},
}];

FIRQuery *query = [[[collRef queryWhereFieldPath:[FIRFieldPath documentID]
isGreaterThan:@"__id7__"]
queryWhereFieldPath:[FIRFieldPath documentID]
isLessThanOrEqualTo:@"A"] queryOrderedByFieldPath:[FIRFieldPath documentID]];
NSArray<NSString *> *expectedDocs =
@[ @"__id12__", @"__id9223372036854775807__", @"12", @"7", @"A" ];
FIRQuerySnapshot *getSnapshot = [self readDocumentSetForRef:query];
XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(getSnapshot), expectedDocs);

id<FIRListenerRegistration> registration =
[query addSnapshotListener:self.eventAccumulator.valueEventHandler];
FIRQuerySnapshot *watchSnapshot = [self.eventAccumulator awaitEventWithName:@"Snapshot"];
XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(watchSnapshot), expectedDocs);

[registration remove];
}

- (void)testSdkOrdersQueryByDocumentIdTheSameWayOnlineAndOffline {
FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
@"A" : @{@"a" : @1},
@"a" : @{@"a" : @1},
@"Aa" : @{@"a" : @1},
@"7" : @{@"a" : @1},
@"12" : @{@"a" : @1},
@"__id7__" : @{@"a" : @1},
@"__id12__" : @{@"a" : @1},
@"__id-2__" : @{@"a" : @1},
@"__id1_" : @{@"a" : @1},
@"_id1__" : @{@"a" : @1},
@"__id9223372036854775807__" : @{@"a" : @1},
@"__id-9223372036854775808__" : @{@"a" : @1},
}];

[self checkOnlineAndOfflineQuery:[collRef queryOrderedByFieldPath:[FIRFieldPath documentID]]
matchesResult:@[
@"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__",
@"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id1_", @"_id1__",
@"a"
]];
}

- (void)testCollectionGroupQueriesWithWhereFiltersOnArbitraryDocumentIDs {
// Use .document() to get a random collection group name to use but ensure it starts with 'b'
// for predictable ordering.
Expand Down
37 changes: 36 additions & 1 deletion Firestore/core/src/model/base_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,18 @@ class BasePath {
std::equal(begin(), end(), potential_child.begin());
}

/**
* Compares 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.
*/
util::ComparisonResult CompareTo(const T& rhs) const {
return util::CompareContainer(segments_, rhs.segments_);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CompareContainer is a generic method that compares items in 2 T& variables by iteration, and it is only used here in base_path. The sorting order is unique to base_path(specifically resource_path), makes more sense to add the logic here instead of modifying the CompareContainer function.

size_t min_size = std::min(size(), rhs.size());
for (size_t i = 0; i < min_size; ++i) {
auto cmp = compareSegments(segments_[i], rhs.segments_[i]);
if (!util::Same(cmp)) return cmp;
}
return util::Compare(size(), rhs.size());
}

friend bool operator==(const BasePath& lhs, const BasePath& rhs) {
Expand All @@ -174,6 +184,31 @@ class BasePath {

private:
SegmentsT segments_;

static util::ComparisonResult compareSegments(const std::string& lhs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C++ style guide suggests function names to start with a capital letter (CompareSegments)

const std::string& rhs) {
bool isLhsNumeric = isNumericId(lhs);
bool isRhsNumeric = isNumericId(rhs);

if (isLhsNumeric && !isRhsNumeric) {
return util::ComparisonResult::Ascending;
} else if (!isLhsNumeric && isRhsNumeric) {
return util::ComparisonResult::Descending;
} else if (isLhsNumeric && isRhsNumeric) {
return util::Compare(extractNumericId(lhs), extractNumericId(rhs));
} else {
return util::Compare(lhs, rhs);
}
}

static bool isNumericId(const std::string& segment) {
return segment.substr(0, 4) == "__id" &&
segment.substr(segment.size() - 2) == "__";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

substr throws and exception if pos is larger than the string length (source). Also not sure what the behavior would be if segment.size is 1 (and therefore segment.size()-2 will be -1). Please add a unit test for these corner cases.

You may want to do:

return
  segment.size() > 6 &&
  segment.substr(0, 4) == "__id" &&
  segment.substr(segment.size() - 2) == "__";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by adding unit tests I mean testing that the output of isNumericId is false for ("__id" or "__id__" or "") for example.

}

static int64_t extractNumericId(const std::string& segment) {
return std::stol(segment.substr(4, segment.size() - 2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::stol throws an std::invalid_argument exception if the argument is an empty string. So it reinforces that this function should not be called if the segment length is 6 or smaller.

}
};

} // namespace impl
Expand Down
Loading