Skip to content

Commit 51d1ac8

Browse files
wu-huiwu-hui
committed
fix: support robust native and BSON subtype 0 binary type ordering and comparison on iOS
1 parent 4239dba commit 51d1ac8

14 files changed

Lines changed: 150 additions & 95 deletions

File tree

‎Firestore/Example/Tests/API/FIRBsonTypesUnitTests.mm‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,9 @@ - (void)testCreateAndReadAndCompareBlob {
158158

159159
// Test reading properties back
160160
XCTAssertEqual(0, std1.subtype);
161-
XCTAssertFalse(std1.isBSON);
162161
XCTAssertEqual(data1, std1.bytes);
163162

164163
XCTAssertEqual(128, bson1.subtype);
165-
XCTAssertTrue(bson1.isBSON);
166164
XCTAssertEqual(data1, bson1.bytes);
167165

168166
// Test isEqual

‎Firestore/Source/API/FIRBlob.m‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,26 +18,25 @@
1818

1919
@implementation FIRBlob
2020

21-
- (instancetype)initWithBytes:(NSData *)bytes subtype:(uint8_t)subtype isBSON:(BOOL)isBSON {
21+
- (instancetype)initWithBytes:(NSData *)bytes subtype:(uint8_t)subtype {
2222
self = [super init];
2323
if (self) {
2424
_subtype = subtype;
2525
_bytes = [bytes copy];
26-
_BSON = isBSON;
2726
}
2827
return self;
2928
}
3029

3130
+ (instancetype)blobWithBytes:(NSData *)bytes {
32-
return [[FIRBlob alloc] initWithBytes:bytes subtype:0 isBSON:NO];
31+
return [[FIRBlob alloc] initWithBytes:bytes subtype:0];
3332
}
3433

3534
+ (instancetype)blobWithBSONBinary:(NSData *)bytes {
36-
return [[FIRBlob alloc] initWithBytes:bytes subtype:0 isBSON:YES];
35+
return [[FIRBlob alloc] initWithBytes:bytes subtype:0];
3736
}
3837

3938
+ (instancetype)blobWithBSONBinary:(NSData *)bytes subtype:(uint8_t)subtype {
40-
return [[FIRBlob alloc] initWithBytes:bytes subtype:subtype isBSON:YES];
39+
return [[FIRBlob alloc] initWithBytes:bytes subtype:subtype];
4140
}
4241

4342
- (BOOL)isEqual:(nullable id)object {
@@ -58,7 +57,7 @@ - (NSUInteger)hash {
5857
}
5958

6059
- (id)copyWithZone:(__unused NSZone *_Nullable)zone {
61-
return [[FIRBlob alloc] initWithBytes:self.bytes subtype:self.subtype isBSON:self.isBSON];
60+
return [[FIRBlob alloc] initWithBytes:self.bytes subtype:self.subtype];
6261
}
6362

6463
- (NSComparisonResult)compare:(FIRBlob *)other {
@@ -87,9 +86,8 @@ - (NSComparisonResult)compare:(FIRBlob *)other {
8786
}
8887

8988
- (NSString *)description {
90-
return [NSString stringWithFormat:@"<FIRBlob: isBSON:%@, subtype:%u, bytes:%@>",
91-
self.isBSON ? @"YES" : @"NO", (unsigned int)self.subtype,
92-
self.bytes];
89+
return [NSString
90+
stringWithFormat:@"<FIRBlob: subtype:%u, bytes:%@>", (unsigned int)self.subtype, self.bytes];
9391
}
9492

9593
@end

‎Firestore/Source/API/FSTUserDataReader.mm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,7 @@ - (ParsedUpdateData)parsedUpdateData:(id)input {
516516
}
517517

518518
- (Message<google_firestore_v1_Value>)parseBlob:(FIRBlob *)blob context:(ParseContext &&)context {
519-
if (blob.isBSON) {
519+
if (blob.subtype != 0) {
520520
uint8_t subtypeByte = blob.subtype;
521521
NSData *data = blob.bytes;
522522

‎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/Source/Public/FirebaseFirestore/FIRBlob.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,6 @@ __attribute__((objc_subclassing_restricted))
3232
/** The binary data. */
3333
@property(nonatomic, copy, readonly) NSData *bytes;
3434

35-
/** True if this Blob represents BSON binary data. */
36-
@property(nonatomic, readonly, getter=isBSON) BOOL BSON;
37-
3835
/** :nodoc: */
3936
- (instancetype)init NS_UNAVAILABLE;
4037

‎Firestore/Swift/Source/Codable/Blob+Codable.swift‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
private protocol CodableBlob: Codable {
3232
var subtype: UInt8 { get }
3333
var bytes: Data { get }
34-
var isBSON: Bool { get }
3534

3635
init(bytes: Data)
3736
init(bsonBinary: Data, subtype: UInt8)
@@ -41,16 +40,14 @@ private protocol CodableBlob: Codable {
4140
private enum BlobKeys: String, CodingKey {
4241
case subtype
4342
case bytes
44-
case isBSON
4543
}
4644

4745
extension CodableBlob {
4846
public init(from decoder: Decoder) throws {
4947
let container = try decoder.container(keyedBy: BlobKeys.self)
5048
let subtype = try container.decode(UInt8.self, forKey: .subtype)
5149
let bytes = try container.decode(Data.self, forKey: .bytes)
52-
let isBSON = try container.decodeIfPresent(Bool.self, forKey: .isBSON) ?? false
53-
if isBSON {
50+
if subtype != 0 {
5451
self.init(bsonBinary: bytes, subtype: subtype)
5552
} else {
5653
self.init(bytes: bytes)
@@ -61,7 +58,6 @@ extension CodableBlob {
6158
var container = encoder.container(keyedBy: BlobKeys.self)
6259
try container.encode(subtype, forKey: .subtype)
6360
try container.encode(bytes, forKey: .bytes)
64-
try container.encode(isBSON, forKey: .isBSON)
6561
}
6662
}
6763

‎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/index/firestore_index_value_writer.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414
* limitations under the License.
1515
*/
1616

17-
#ifndef FIRESTORE_CORE_SRC_INDEX_FIRESTORE_INDEX_VALUE_WRITER_H_
18-
#define FIRESTORE_CORE_SRC_INDEX_FIRESTORE_INDEX_VALUE_WRITER_H_
17+
#ifndef FIREBASE_IOS_SDK_FIRESTORE_CORE_SRC_INDEX_FIRESTORE_INDEX_VALUE_WRITER_H_
18+
#define FIREBASE_IOS_SDK_FIRESTORE_CORE_SRC_INDEX_FIRESTORE_INDEX_VALUE_WRITER_H_
1919

2020
#include "Firestore/core/src/index/index_byte_encoder.h"
2121
#include "Firestore/core/src/nanopb/nanopb_util.h"
@@ -61,4 +61,4 @@ void WriteIndexValue(const google_firestore_v1_Value& value,
6161
} // namespace firestore
6262
} // namespace firebase
6363

64-
#endif // FIRESTORE_CORE_SRC_INDEX_FIRESTORE_INDEX_VALUE_WRITER_H_
64+
#endif // FIREBASE_IOS_SDK_FIRESTORE_CORE_SRC_INDEX_FIRESTORE_INDEX_VALUE_WRITER_H_

‎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: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414
* limitations under the License.
1515
*/
1616

17-
#ifndef FIRESTORE_CORE_SRC_MODEL_VALUE_UTIL_H_
18-
#define FIRESTORE_CORE_SRC_MODEL_VALUE_UTIL_H_
17+
#ifndef FIREBASE_IOS_SDK_FIRESTORE_CORE_SRC_MODEL_VALUE_UTIL_H_
18+
#define FIREBASE_IOS_SDK_FIRESTORE_CORE_SRC_MODEL_VALUE_UTIL_H_
1919

2020
#include <ostream>
2121
#include <string>
@@ -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
/**
@@ -490,4 +489,4 @@ inline std::ostream& operator<<(std::ostream& out,
490489
} // namespace firestore
491490
} // namespace firebase
492491

493-
#endif // FIRESTORE_CORE_SRC_MODEL_VALUE_UTIL_H_
492+
#endif // FIREBASE_IOS_SDK_FIRESTORE_CORE_SRC_MODEL_VALUE_UTIL_H_

0 commit comments

Comments
 (0)