Skip to content

Commit 131cd8c

Browse files
wu-huiwu-hui
committed
fix: support robust native and BSON subtype 0 binary type ordering and comparison on iOS
1 parent d27c90b commit 131cd8c

8 files changed

Lines changed: 135 additions & 69 deletions

File tree

‎Firestore/Source/API/FSTUserDataWriter.mm‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ - (id)convertedValue:(const google_firestore_v1_Value &)value {
125125
case TypeOrder::kString:
126126
return MakeNSString(MakeStringView(value.string_value));
127127
case TypeOrder::kBlob:
128+
if (value.which_value_type == google_firestore_v1_Value_map_value_tag) {
129+
return [self convertedBsonBinaryData:value.map_value];
130+
}
128131
return MakeNSData(value.bytes_value);
129132
case TypeOrder::kGeoPoint:
130133
return MakeFIRGeoPoint(
@@ -139,8 +142,6 @@ - (id)convertedValue:(const google_firestore_v1_Value &)value {
139142
return [self convertedBsonObjectId:value.map_value];
140143
case TypeOrder::kBsonTimestamp:
141144
return [self convertedBsonTimestamp:value.map_value];
142-
case TypeOrder::kBsonBinaryData:
143-
return [self convertedBsonBinaryData:value.map_value];
144145
case TypeOrder::kVector:
145146
return [self convertedVector:value.map_value];
146147
case TypeOrder::kInternalMaxValue:

‎Firestore/core/src/index/firestore_index_value_writer.cc‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,21 @@ void WriteIndexMap(google_firestore_v1_MapValue map_index_value,
133133
void WriteIndexBsonBinaryData(
134134
const google_firestore_v1_MapValue& map_index_value,
135135
DirectionalIndexByteEncoder* encoder) {
136-
WriteValueTypeLabel(encoder, IndexType::kBsonBinaryData);
137-
encoder->WriteBytes(map_index_value.fields[0].value.bytes_value);
136+
const pb_bytes_array_t* bytes = map_index_value.fields[0].value.bytes_value;
137+
if (!bytes || bytes->size == 0) {
138+
WriteValueTypeLabel(encoder, IndexType::kBlob);
139+
encoder->WriteBytes(const_cast<pb_bytes_array_t*>(bytes));
140+
WriteTruncationMarker(encoder);
141+
return;
142+
}
143+
uint8_t subtype = bytes->bytes[0];
144+
if (subtype == 0) {
145+
WriteValueTypeLabel(encoder, IndexType::kBlob);
146+
encoder->WriteString(nanopb::MakeStringView(bytes).substr(1));
147+
} else {
148+
WriteValueTypeLabel(encoder, IndexType::kBsonBinaryData);
149+
encoder->WriteBytes(const_cast<pb_bytes_array_t*>(bytes));
150+
}
138151
WriteTruncationMarker(encoder);
139152
}
140153

‎Firestore/core/src/model/value_util.cc‎

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ TypeOrder GetTypeOrder(const google_firestore_v1_Value& value) {
219219
case MapType::kBsonTimestamp:
220220
return TypeOrder::kBsonTimestamp;
221221
case MapType::kBsonBinaryData:
222-
return TypeOrder::kBsonBinaryData;
222+
return TypeOrder::kBlob;
223223
case MapType::kNormal:
224224
default:
225225
return TypeOrder::kMap;
@@ -348,21 +348,41 @@ ComparisonResult CompareStrings(const google_firestore_v1_Value& left,
348348
return util::Compare(left_string, right_string);
349349
}
350350

351+
int GetBinarySubtype(const google_firestore_v1_Value& value) {
352+
if (value.which_value_type == google_firestore_v1_Value_bytes_value_tag) {
353+
return 0;
354+
}
355+
HARD_ASSERT(IsBsonBinaryData(value), "Expected binary value");
356+
const pb_bytes_array_t* bytes = value.map_value.fields[0].value.bytes_value;
357+
if (!bytes || bytes->size == 0) {
358+
return 0;
359+
}
360+
return bytes->bytes[0];
361+
}
362+
363+
absl::string_view GetBinaryData(const google_firestore_v1_Value& value) {
364+
if (value.which_value_type == google_firestore_v1_Value_bytes_value_tag) {
365+
if (!value.bytes_value) {
366+
return {};
367+
}
368+
return nanopb::MakeStringView(value.bytes_value);
369+
}
370+
HARD_ASSERT(IsBsonBinaryData(value), "Expected binary value");
371+
const pb_bytes_array_t* bytes = value.map_value.fields[0].value.bytes_value;
372+
if (!bytes || bytes->size <= 1) {
373+
return {};
374+
}
375+
return nanopb::MakeStringView(bytes).substr(1);
376+
}
377+
351378
ComparisonResult CompareBlobs(const google_firestore_v1_Value& left,
352379
const google_firestore_v1_Value& right) {
353-
if (left.bytes_value && right.bytes_value) {
354-
size_t size = std::min(left.bytes_value->size, right.bytes_value->size);
355-
int cmp =
356-
std::memcmp(left.bytes_value->bytes, right.bytes_value->bytes, size);
357-
return cmp != 0
358-
? util::ComparisonResultFromInt(cmp)
359-
: util::Compare(left.bytes_value->size, right.bytes_value->size);
360-
} else {
361-
// An empty blob is represented by a nullptr (or an empty byte array)
362-
return util::Compare(
363-
!(left.bytes_value == nullptr || left.bytes_value->size == 0),
364-
!(right.bytes_value == nullptr || right.bytes_value->size == 0));
380+
int left_subtype = GetBinarySubtype(left);
381+
int right_subtype = GetBinarySubtype(right);
382+
if (left_subtype != right_subtype) {
383+
return util::Compare(left_subtype, right_subtype);
365384
}
385+
return util::Compare(GetBinaryData(left), GetBinaryData(right));
366386
}
367387

368388
ComparisonResult CompareReferences(const google_firestore_v1_Value& left,
@@ -642,9 +662,6 @@ ComparisonResult Compare(const google_firestore_v1_Value& left,
642662
case TypeOrder::kBsonTimestamp:
643663
return CompareBsonTimestamp(left, right);
644664

645-
case TypeOrder::kBsonBinaryData:
646-
return CompareBsonBinaryData(left, right);
647-
648665
case TypeOrder::kArray:
649666
return CompareArrays(left, right);
650667

@@ -796,9 +813,6 @@ bool Equals(const google_firestore_v1_Value& lhs,
796813
case TypeOrder::kBsonTimestamp:
797814
return CompareBsonTimestamp(lhs, rhs) == ComparisonResult::Same;
798815

799-
case TypeOrder::kBsonBinaryData:
800-
return CompareBsonBinaryData(lhs, rhs) == ComparisonResult::Same;
801-
802816
case TypeOrder::kVector:
803817
case TypeOrder::kMap:
804818
case TypeOrder::kInternalMaxValue:
@@ -959,7 +973,7 @@ google_firestore_v1_Value GetLowerBound(
959973
} else if (IsBsonTimestamp(value)) {
960974
return MinBsonTimestamp();
961975
} else if (IsBsonBinaryData(value)) {
962-
return MinBsonBinaryData();
976+
return MinBytes();
963977
} else if (IsRegexValue(value)) {
964978
return MinRegex();
965979
} else if (IsInt32Value(value) || IsDecimal128Value(value)) {
@@ -995,7 +1009,7 @@ google_firestore_v1_Value GetUpperBound(
9951009
case google_firestore_v1_Value_string_value_tag:
9961010
return MinBytes();
9971011
case google_firestore_v1_Value_bytes_value_tag:
998-
return MinBsonBinaryData();
1012+
return MinReference();
9991013
case google_firestore_v1_Value_reference_value_tag:
10001014
return MinBsonObjectId();
10011015
case google_firestore_v1_Value_geo_point_value_tag:

‎Firestore/core/src/model/value_util.h‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -123,16 +123,15 @@ enum class TypeOrder {
123123
kServerTimestamp = 6,
124124
kString = 7,
125125
kBlob = 8,
126-
kBsonBinaryData = 9,
127-
kReference = 10,
128-
kBsonObjectId = 11,
129-
kGeoPoint = 12,
130-
kRegex = 13,
131-
kArray = 14,
132-
kVector = 15,
133-
kMap = 16,
134-
kMaxKey = 17,
135-
kInternalMaxValue = 18
126+
kReference = 9,
127+
kBsonObjectId = 10,
128+
kGeoPoint = 11,
129+
kRegex = 12,
130+
kArray = 13,
131+
kVector = 14,
132+
kMap = 15,
133+
kMaxKey = 16,
134+
kInternalMaxValue = 17
136135
};
137136

138137
/**

‎Firestore/core/test/unit/index/index_value_writer_test.cc‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,21 @@ TEST(IndexValueWriterTest, writeIndexValueSupportsBsonBinaryData) {
142142
EXPECT_EQ(actual_bytes, expected_bytes);
143143
}
144144

145+
TEST(IndexValueWriterTest, writeIndexValueSupportsBsonBinarySubtype0) {
146+
auto bson_value = BsonBinaryData(0, {1, 2, 3});
147+
auto blob_value = testutil::BlobValue(1, 2, 3);
148+
149+
IndexEncodingBuffer bson_encoder;
150+
WriteIndexValue(*bson_value,
151+
bson_encoder.ForKind(model::Segment::Kind::kAscending));
152+
153+
IndexEncodingBuffer blob_encoder;
154+
WriteIndexValue(*blob_value,
155+
blob_encoder.ForKind(model::Segment::Kind::kAscending));
156+
157+
EXPECT_EQ(bson_encoder.GetEncodedBytes(), blob_encoder.GetEncodedBytes());
158+
}
159+
145160
TEST(IndexValueWriterTest, writeIndexValueSupportsBsonBinaryWithEmptyData) {
146161
// Value
147162
auto value = BsonBinaryData(1, {});

‎Firestore/core/test/unit/local/leveldb_index_manager_test.cc‎

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1149,6 +1149,40 @@ TEST_F(LevelDbIndexManagerTest, IndexBsonBinaryDataFields) {
11491149
});
11501150
}
11511151

1152+
TEST_F(LevelDbIndexManagerTest, IndexBlobAndBsonBinaryOrderingAndMatching) {
1153+
persistence_->Run("TestIndexBlobAndBsonBinaryOrderingAndMatching", [&]() {
1154+
index_manager_->Start();
1155+
index_manager_->AddFieldIndex(
1156+
MakeFieldIndex("coll", "key", model::Segment::kAscending));
1157+
index_manager_->AddFieldIndex(
1158+
MakeFieldIndex("coll", "key", model::Segment::kDescending));
1159+
1160+
AddDoc("coll/doc_blob", Map("key", testutil::BlobValue(1, 2, 3)));
1161+
AddDoc("coll/doc_bson0", Map("key", BsonBinaryData(0, {1, 2, 3})));
1162+
AddDoc("coll/doc_bson1", Map("key", BsonBinaryData(1, {1, 2, 3})));
1163+
1164+
auto asc_query = Query("coll").AddingOrderBy(OrderBy("key", "asc"));
1165+
VerifyResults(asc_query,
1166+
{"coll/doc_blob", "coll/doc_bson0", "coll/doc_bson1"});
1167+
1168+
auto desc_query = Query("coll").AddingOrderBy(OrderBy("key", "desc"));
1169+
VerifyResults(desc_query,
1170+
{"coll/doc_bson1", "coll/doc_bson0", "coll/doc_blob"});
1171+
1172+
auto blob_query = Query("coll").AddingFilter(
1173+
Filter("key", "==", testutil::BlobValue(1, 2, 3)));
1174+
VerifyResults(blob_query, {"coll/doc_bson0", "coll/doc_blob"});
1175+
1176+
auto bson0_query = Query("coll").AddingFilter(
1177+
Filter("key", "==", BsonBinaryData(0, {1, 2, 3})));
1178+
VerifyResults(bson0_query, {"coll/doc_bson0", "coll/doc_blob"});
1179+
1180+
auto bson1_query = Query("coll").AddingFilter(
1181+
Filter("key", "==", BsonBinaryData(1, {1, 2, 3})));
1182+
VerifyResults(bson1_query, {"coll/doc_bson1"});
1183+
});
1184+
}
1185+
11521186
TEST_F(LevelDbIndexManagerTest, IndexBsonTimestampFields) {
11531187
persistence_->Run("TestIndexBsonTimestampFields", [&]() {
11541188
index_manager_->Start();
@@ -1657,6 +1691,7 @@ TEST_F(LevelDbIndexManagerTest, IndexAllTypesTogether) {
16571691
AddDoc("coll/doc15", Map("key", BsonTimestamp(1, 2)));
16581692
AddDoc("coll/doc16", Map("key", "string"));
16591693
AddDoc("coll/doc17", Map("key", BlobValue(0, 1, 255)));
1694+
AddDoc("coll/doc17_bson0", Map("key", BsonBinaryData(0, {0, 1, 255})));
16601695
AddDoc("coll/doc18", Map("key", BsonBinaryData(1, {1, 2, 3})));
16611696
AddDoc("coll/doc19", Map("key", Ref("project", "coll/doc")));
16621697
AddDoc("coll/doc20", Map("key", BsonObjectId("507f191e810c19729de860ea")));
@@ -1669,14 +1704,14 @@ TEST_F(LevelDbIndexManagerTest, IndexAllTypesTogether) {
16691704

16701705
auto query = Query("coll").AddingOrderBy(OrderBy("key", "desc"));
16711706

1672-
VerifyResults(
1673-
query,
1674-
{"coll/doc26", "coll/doc25", "coll/doc24", "coll/doc23", "coll/doc22",
1675-
"coll/doc21", "coll/doc20", "coll/doc19", "coll/doc18", "coll/doc17",
1676-
"coll/doc16", "coll/doc15", "coll/doc14", "coll/doc13", "coll/doc12",
1677-
"coll/doc11", "coll/doc10", "coll/doc9", "coll/doc8", "coll/doc7",
1678-
"coll/doc6", "coll/doc5", "coll/doc4", "coll/doc3", "coll/doc2",
1679-
"coll/doc1"});
1707+
VerifyResults(query,
1708+
{"coll/doc26", "coll/doc25", "coll/doc24", "coll/doc23",
1709+
"coll/doc22", "coll/doc21", "coll/doc20", "coll/doc19",
1710+
"coll/doc18", "coll/doc17_bson0", "coll/doc17", "coll/doc16",
1711+
"coll/doc15", "coll/doc14", "coll/doc13", "coll/doc12",
1712+
"coll/doc11", "coll/doc10", "coll/doc9", "coll/doc8",
1713+
"coll/doc7", "coll/doc6", "coll/doc5", "coll/doc4",
1714+
"coll/doc3", "coll/doc2", "coll/doc1"});
16801715
});
16811716
}
16821717

‎Firestore/core/test/unit/model/value_util_test.cc‎

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ TEST(FieldValueTest, ValueHelpers) {
236236
ASSERT_EQ(DetectMapType(*bson_timestamp_value), MapType::kBsonTimestamp);
237237

238238
auto bson_binary_data_value = BsonBinaryData(1, {1, 2, 3});
239-
ASSERT_EQ(GetTypeOrder(*bson_binary_data_value), TypeOrder::kBsonBinaryData);
239+
ASSERT_EQ(GetTypeOrder(*bson_binary_data_value), TypeOrder::kBlob);
240240
ASSERT_EQ(DetectMapType(*bson_binary_data_value), MapType::kBsonBinaryData);
241241
}
242242

@@ -315,6 +315,7 @@ TEST_F(ValueUtilTest, Equality) {
315315
Add(equals_group, Decimal128("Infinity"));
316316
Add(equals_group, BlobValue(0, 1, 1));
317317
Add(equals_group, BlobValue(0, 1));
318+
Add(equals_group, BlobValue(7, 8, 9), BsonBinaryData(0, {7, 8, 9}));
318319
Add(equals_group, "string", "string");
319320
Add(equals_group, "strin");
320321
Add(equals_group, std::string("strin\0", 6));
@@ -425,15 +426,12 @@ TEST_F(ValueUtilTest, StrictOrdering) {
425426
// latin small letter e with acute accent + latin small letter a
426427
Add(comparison_groups, "\u00e9a");
427428

428-
// blobs
429-
Add(comparison_groups, BlobValue());
429+
// blobs (native & BSON binary merged)
430+
Add(comparison_groups, BlobValue(), DeepClone(MinBsonBinaryData()));
430431
Add(comparison_groups, BlobValue(0));
431432
Add(comparison_groups, BlobValue(0, 1, 2, 3, 4));
432433
Add(comparison_groups, BlobValue(0, 1, 2, 4, 3));
433434
Add(comparison_groups, BlobValue(255));
434-
435-
// BSON Binary Data
436-
Add(comparison_groups, DeepClone(MinBsonBinaryData()));
437435
Add(comparison_groups, BsonBinaryData(5, {1, 2, 3}),
438436
BsonBinaryData(5, {1, 2, 3}));
439437
Add(comparison_groups, BsonBinaryData(7, {1}));
@@ -589,16 +587,13 @@ TEST_F(ValueUtilTest, RelaxedOrdering) {
589587
Add(comparison_groups, "\u00e9a");
590588
Add(comparison_groups, DeepClone(MinBytes()));
591589

592-
// blobs
590+
// blobs (native & BSON binary merged)
593591
Add(comparison_groups, DeepClone(MinBytes()));
594-
Add(comparison_groups, BlobValue());
592+
Add(comparison_groups, BlobValue(), DeepClone(MinBsonBinaryData()));
595593
Add(comparison_groups, BlobValue(0));
596594
Add(comparison_groups, BlobValue(0, 1, 2, 3, 4));
597595
Add(comparison_groups, BlobValue(0, 1, 2, 4, 3));
598596
Add(comparison_groups, BlobValue(255));
599-
600-
// BSON Binary Data
601-
Add(comparison_groups, DeepClone(MinBsonBinaryData()));
602597
Add(comparison_groups, BsonBinaryData(5, {1, 2, 3}),
603598
BsonBinaryData(5, {1, 2, 3}));
604599
Add(comparison_groups, BsonBinaryData(7, {1}));
@@ -720,15 +715,11 @@ TEST_F(ValueUtilTest, ComputesLowerBound) {
720715
Add(groups, GetLowerBoundMessage(Value("Z")), "", DeepClone(MinString()));
721716
Add(groups, "\u0000");
722717

723-
// Blobs
724-
Add(groups, GetLowerBoundMessage(BlobValue(1, 2, 3)), BlobValue(),
725-
DeepClone(MinBytes()));
726-
Add(groups, BlobValue(0));
727-
728-
// BSON Binary Data
729-
Add(groups, GetLowerBoundMessage(BsonBinaryData(128, {128, 128})),
730-
DeepClone(MinBsonBinaryData()));
731-
Add(groups, BsonBinaryData(0, {0}));
718+
// Blobs (native & BSON binary merged)
719+
Add(groups, GetLowerBoundMessage(BlobValue(1, 2, 3)),
720+
GetLowerBoundMessage(BsonBinaryData(128, {128, 128})), BlobValue(),
721+
DeepClone(MinBytes()), DeepClone(MinBsonBinaryData()));
722+
Add(groups, BlobValue(0), BsonBinaryData(0, {0}));
732723

733724
// References
734725
Add(groups, GetLowerBoundMessage(RefValue(DbId("p1/d1"), Key("c1/doc1"))),
@@ -818,13 +809,11 @@ TEST_F(ValueUtilTest, ComputesUpperBound) {
818809
Add(groups, "\u0000");
819810
Add(groups, GetUpperBoundMessage(DeepClone(MinString())));
820811

821-
// Blobs
812+
// Blobs (native & BSON binary merged)
822813
Add(groups, BlobValue(255));
823-
Add(groups, GetUpperBoundMessage(BlobValue()));
824-
825-
// BSON Binary Data
826814
Add(groups, BsonBinaryData(255, {255, 255}));
827-
Add(groups, GetUpperBoundMessage(DeepClone(MinBsonBinaryData())));
815+
Add(groups, GetUpperBoundMessage(BlobValue()),
816+
GetUpperBoundMessage(DeepClone(MinBsonBinaryData())));
828817

829818
// References
830819
Add(groups, DeepClone(MinReference()));

‎Firestore/core/test/unit/remote/serializer_test.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -930,7 +930,7 @@ TEST_F(SerializerTest, EncodesBsonBinaryData) {
930930
(*fields)["__binary__"] =
931931
ValueProto(ByteString(concat.data(), concat.size()));
932932

933-
ExpectRoundTrip(model, proto, TypeOrder::kBsonBinaryData);
933+
ExpectRoundTrip(model, proto, TypeOrder::kBlob);
934934
}
935935

936936
TEST_F(SerializerTest, EncodesVectorValue) {

0 commit comments

Comments
 (0)