Skip to content

Commit 04d1d41

Browse files
wilhuffwilhuff
authored
Migrate FSTBlobValue to FSTDelegateValue (#3130)
* Rename nanopb::String to nanopb::ByteString ... and flesh it out. * Remove const pb_bytes_array* constructor * Use std:: prefixes * Pull forward CopyVector * Simplify nanopb::Reader/Writer construction * Reuse ByteString construction in serializer * Add nanopb_testing.h * Make QueryData.resume_token a ByteString * Use ByteString as FieldValue::blob_value * Migrate FSTBlobValue to FSTDelegateValue * Rework the ByteString API * Make data(), begin(), and end() never return nullptr * Make copies of pb_bytes_array_t* more obvious (also avoids ambiguity with the private constructor). * Remove constructors that duplicate absl::string_view * Make ByteString::Copy just a constructor * Remove EncodeBytes/DecodeBytes These duplicate ByteString constructors * Overhaul ByteString and users * Remove most conversions from ByteString * Put remaining conversions in nanopb_util as free functions * Make ByteStringWriter accumulate in a pb_bytes_array_t* * Have ProtobufSerialize use ByteStringWriter * Avoid reserving/writing if we're not asked to append data * Ensure ByteStringWriter initializes/frees * Make copies explicit * Make Reserve match std::vector::reserve * Add comments, assertions, and tests
1 parent 5538146 commit 04d1d41

37 files changed

+625
-378
lines changed

‎Firestore/Example/Firestore.xcodeproj/project.pbxproj‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@
136136
3A7CB01751697ED599F2D9A1 /* executor_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6FB4688208F9B9100554BA2 /* executor_test.cc */; };
137137
3B47E82ED2A3C59AB5002640 /* FSTMemoryLocalStoreTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E0882021552A00B64F25 /* FSTMemoryLocalStoreTests.mm */; };
138138
3B843E4C1F3A182900548890 /* remote_store_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 3B843E4A1F3930A400548890 /* remote_store_spec_test.json */; };
139+
3BA4EEA6153B3833F86B8104 /* writer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = BC3C788D290A935C353CEAA1 /* writer_test.cc */; };
139140
3BCEBA50E9678123245C0272 /* empty_credentials_provider_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB38D93620239689000A432D /* empty_credentials_provider_test.cc */; };
140141
3D11B104A8F01F85180B38F6 /* field_transform_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 54A0352320A3AEC3003E0143 /* field_transform_test.mm */; };
141142
3D9619906F09108E34FF0C95 /* FSTSmokeTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07C202154EB00B64F25 /* FSTSmokeTests.mm */; };
@@ -326,6 +327,7 @@
326327
58E377DCCC64FE7D2C6B59A1 /* database_id_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB71064B201FA60300344F18 /* database_id_test.cc */; };
327328
596C782EFB68131380F8EEF8 /* user_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB38D93220239654000A432D /* user_test.cc */; };
328329
59D1E0A722CE68E00A3F85AA /* FSTLevelDBTransactionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 132E36BB104830BD806351AC /* FSTLevelDBTransactionTests.mm */; };
330+
59E6941008253D4B0F77C2BA /* writer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = BC3C788D290A935C353CEAA1 /* writer_test.cc */; };
329331
5A080105CCBFDB6BF3F3772D /* path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 403DBF6EFB541DFD01582AA3 /* path_test.cc */; };
330332
5B62003FEA9A3818FDF4E2DD /* document_key_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6152AD5202A5385000E5744 /* document_key_test.cc */; };
331333
5BE49546D57C43DDFCDB6FBD /* to_string_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = B68B1E002213A764008977EF /* to_string_apple_test.mm */; };
@@ -466,6 +468,7 @@
466468
9D71628E38D9F64C965DF29E /* FSTAPIHelpers.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04E202154AA00B64F25 /* FSTAPIHelpers.mm */; };
467469
A17DBC8F24127DA8A381F865 /* testutil.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54A0352820A3B3BD003E0143 /* testutil.cc */; };
468470
A1F57CC739211F64F2E9232D /* hard_assert_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 444B7AB3F5A2929070CB1363 /* hard_assert_test.cc */; };
471+
A21819C437C3C80450D7EEEE /* writer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = BC3C788D290A935C353CEAA1 /* writer_test.cc */; };
469472
A38F4AE525A87FDEA41DED47 /* FSTLevelDBQueryCacheTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E0982021552C00B64F25 /* FSTLevelDBQueryCacheTests.mm */; };
470473
A4ECA8335000CBDF94586C94 /* FSTDatastoreTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E07E202154EC00B64F25 /* FSTDatastoreTests.mm */; };
471474
A55266E6C986251D283CE948 /* FIRCursorTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E070202154D600B64F25 /* FIRCursorTests.mm */; };
@@ -1032,6 +1035,7 @@
10321035
B9C261C26C5D311E1E3C0CB9 /* query_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = query_test.cc; sourceTree = "<group>"; };
10331036
BA6E5B9D53CCF301F58A62D7 /* xcgmock.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = xcgmock.h; sourceTree = "<group>"; };
10341037
BB92EB03E3F92485023F64ED /* Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework; sourceTree = BUILT_PRODUCTS_DIR; };
1038+
BC3C788D290A935C353CEAA1 /* writer_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; name = writer_test.cc; path = nanopb/writer_test.cc; sourceTree = "<group>"; };
10351039
BD01F0E43E4E2A07B8B05099 /* Pods-Firestore_Tests_macOS.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Tests_macOS.debug.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Tests_macOS/Pods-Firestore_Tests_macOS.debug.xcconfig"; sourceTree = "<group>"; };
10361040
C8522DE226C467C54E6788D8 /* mutation_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = mutation_test.cc; sourceTree = "<group>"; };
10371041
CD422AF3E4515FB8E9BE67A0 /* equals_tester.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = equals_tester.h; sourceTree = "<group>"; };
@@ -1404,6 +1408,7 @@
14041408
children = (
14051409
5342CDDB137B4E93E2E85CCA /* byte_string_test.cc */,
14061410
2DAA26538D1A93A39F8AC373 /* nanopb_testing.h */,
1411+
BC3C788D290A935C353CEAA1 /* writer_test.cc */,
14071412
);
14081413
name = nanopb;
14091414
sourceTree = "<group>";
@@ -3134,6 +3139,7 @@
31343139
16F52ECC6FA8A0587CD779EB /* user_test.cc in Sources */,
31353140
E3C0E5F834A82EEE9F8C4519 /* watch_change_test.mm in Sources */,
31363141
53AB47E44D897C81A94031F6 /* write.pb.cc in Sources */,
3142+
59E6941008253D4B0F77C2BA /* writer_test.cc in Sources */,
31373143
2E6E6164F44B9E3C6BB88313 /* xcgmock_test.mm in Sources */,
31383144
);
31393145
runOnlyForDeploymentPostprocessing = 0;
@@ -3309,6 +3315,7 @@
33093315
596C782EFB68131380F8EEF8 /* user_test.cc in Sources */,
33103316
178FE1E277C63B3E7120BE56 /* watch_change_test.mm in Sources */,
33113317
A5AB1815C45FFC762981E481 /* write.pb.cc in Sources */,
3318+
A21819C437C3C80450D7EEEE /* writer_test.cc in Sources */,
33123319
6EB896CD1B64A60E6C82D8CC /* xcgmock_test.mm in Sources */,
33133320
);
33143321
runOnlyForDeploymentPostprocessing = 0;
@@ -3559,6 +3566,7 @@
35593566
ABC1D7DE2023A05300BA84F0 /* user_test.cc in Sources */,
35603567
B68FC0E521F6848700A7055C /* watch_change_test.mm in Sources */,
35613568
544129DE21C2DDC800EFB9CC /* write.pb.cc in Sources */,
3569+
3BA4EEA6153B3833F86B8104 /* writer_test.cc in Sources */,
35623570
9794E074439ABE5457E60F35 /* xcgmock_test.mm in Sources */,
35633571
);
35643572
runOnlyForDeploymentPostprocessing = 0;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ - (void)testConvertsBlobs {
131131
NSArray *values = @[ FSTTestData(1, 2, 3), FSTTestData(1, 2) ];
132132
for (id value in values) {
133133
FSTFieldValue *wrapped = FSTTestFieldValue(value);
134-
XCTAssertEqualObjects([wrapped class], [FSTBlobValue class]);
134+
XCTAssertEqualObjects([wrapped class], [FSTDelegateValue class]);
135135
XCTAssertEqualObjects([wrapped value], value);
136136
XCTAssertEqual(wrapped.type, FieldValue::Type::Blob);
137137
}

‎Firestore/Example/Tests/Model/FSTFieldValueTests.mm‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -298,9 +298,7 @@ - (void)testValueEquality {
298298
// double and unit64_t values can compare: the same (but won't be isEqual:)
299299
@[ FSTTestFieldValue(@1.0), FieldValue::FromDouble(1.0).Wrap() ],
300300
@[ FSTTestFieldValue(@1.1), FieldValue::FromDouble(1.1).Wrap() ],
301-
@[
302-
FSTTestFieldValue(FSTTestData(0, 1, 2, -1)), [FSTBlobValue blobValue:FSTTestData(0, 1, 2, -1)]
303-
],
301+
@[ FSTTestFieldValue(FSTTestData(0, 1, 2, -1)), testutil::BlobValue(0, 1, 2).Wrap() ],
304302
@[ FSTTestFieldValue(FSTTestData(0, 1, -1)) ],
305303
@[ FSTTestFieldValue(@"string"), FieldValue::FromString("string").Wrap() ],
306304
@[ FSTTestFieldValue(@"strin") ],
@@ -323,10 +321,7 @@ - (void)testValueEquality {
323321
@[ [FSTServerTimestampValue
324322
serverTimestampValueWithLocalWriteTime:[FIRTimestamp timestampWithDate:date2]
325323
previousValue:nil] ],
326-
@[
327-
FSTTestFieldValue(FSTTestGeoPoint(0, 1)),
328-
FieldValue::FromGeoPoint(GeoPoint(0, 1)).Wrap()
329-
],
324+
@[ FSTTestFieldValue(FSTTestGeoPoint(0, 1)), FieldValue::FromGeoPoint(GeoPoint(0, 1)).Wrap() ],
330325
@[ FSTTestFieldValue(FSTTestGeoPoint(1, 0)) ],
331326
@[
332327
[FSTReferenceValue

‎Firestore/Source/API/FSTUserDataConverter.mm‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
#include "Firestore/core/src/firebase/firestore/model/field_transform.h"
4343
#include "Firestore/core/src/firebase/firestore/model/precondition.h"
4444
#include "Firestore/core/src/firebase/firestore/model/transform_operations.h"
45+
#include "Firestore/core/src/firebase/firestore/nanopb/nanopb_util.h"
4546
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
4647
#include "Firestore/core/src/firebase/firestore/util/string_apple.h"
4748
#include "absl/memory/memory.h"
@@ -65,6 +66,7 @@
6566
using firebase::firestore::model::Precondition;
6667
using firebase::firestore::model::ServerTimestampTransform;
6768
using firebase::firestore::model::TransformOperation;
69+
using firebase::firestore::nanopb::MakeByteString;
6870

6971
NS_ASSUME_NONNULL_BEGIN
7072

@@ -457,7 +459,8 @@ - (nullable FSTFieldValue *)parseScalarValue:(nullable id)input context:(ParseCo
457459
return FieldValue::FromGeoPoint([geoPoint toGeoPoint]).Wrap();
458460

459461
} else if ([input isKindOfClass:[NSData class]]) {
460-
return [FSTBlobValue blobValue:input];
462+
NSData *inputData = input;
463+
return FieldValue::FromBlob(MakeByteString(inputData)).Wrap();
461464

462465
} else if ([input isKindOfClass:[FSTDocumentKeyReference class]]) {
463466
FSTDocumentKeyReference *reference = input;

‎Firestore/Source/Model/FSTFieldValue.h‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,13 +132,6 @@ typedef NS_ENUM(NSInteger, FSTTypeOrder) {
132132

133133
@end
134134

135-
/**
136-
* A blob value stored in Firestore.
137-
*/
138-
@interface FSTBlobValue : FSTFieldValue <NSData *>
139-
+ (instancetype)blobValue:(NSData *)value;
140-
@end
141-
142135
/**
143136
* A reference value stored in Firestore.
144137
*/

‎Firestore/Source/Model/FSTFieldValue.mm‎

Lines changed: 3 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include "Firestore/core/src/firebase/firestore/model/database_id.h"
3030
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
3131
#include "Firestore/core/src/firebase/firestore/model/field_path.h"
32+
#include "Firestore/core/src/firebase/firestore/nanopb/nanopb_util.h"
3233
#include "Firestore/core/src/firebase/firestore/util/comparison.h"
3334
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
3435
#include "Firestore/core/src/firebase/firestore/util/string_apple.h"
@@ -41,6 +42,7 @@
4142
using firebase::firestore::model::FieldValue;
4243
using firebase::firestore::model::FieldValueOptions;
4344
using firebase::firestore::model::ServerTimestampBehavior;
45+
using firebase::firestore::nanopb::MakeNSData;
4446
using firebase::firestore::util::Comparator;
4547
using firebase::firestore::util::CompareMixedNumber;
4648
using firebase::firestore::util::DoubleBitwiseEquals;
@@ -253,75 +255,6 @@ - (NSComparisonResult)compare:(FSTFieldValue *)other {
253255

254256
@end
255257

256-
#pragma mark - FSTBlobValue
257-
258-
static NSComparisonResult CompareBytes(NSData *left, NSData *right) {
259-
NSUInteger minLength = MIN(left.length, right.length);
260-
int result = memcmp(left.bytes, right.bytes, minLength);
261-
if (result < 0) {
262-
return NSOrderedAscending;
263-
} else if (result > 0) {
264-
return NSOrderedDescending;
265-
} else if (left.length < right.length) {
266-
return NSOrderedAscending;
267-
} else if (left.length > right.length) {
268-
return NSOrderedDescending;
269-
} else {
270-
return NSOrderedSame;
271-
}
272-
}
273-
274-
@interface FSTBlobValue ()
275-
@property(nonatomic, copy, readonly) NSData *internalValue;
276-
@end
277-
278-
// TODO(b/37267885): Add truncation support
279-
@implementation FSTBlobValue
280-
281-
+ (instancetype)blobValue:(NSData *)value {
282-
return [[FSTBlobValue alloc] initWithValue:value];
283-
}
284-
285-
- (id)initWithValue:(NSData *)value {
286-
self = [super init];
287-
if (self) {
288-
_internalValue = [value copy];
289-
}
290-
return self;
291-
}
292-
293-
- (FieldValue::Type)type {
294-
return FieldValue::Type::Blob;
295-
}
296-
297-
- (FSTTypeOrder)typeOrder {
298-
return FSTTypeOrderBlob;
299-
}
300-
301-
- (id)value {
302-
return self.internalValue;
303-
}
304-
305-
- (BOOL)isEqual:(id)other {
306-
return [other isKindOfClass:[FSTFieldValue class]] &&
307-
((FSTFieldValue *)other).type == FieldValue::Type::Blob &&
308-
[self.internalValue isEqual:((FSTBlobValue *)other).internalValue];
309-
}
310-
311-
- (NSUInteger)hash {
312-
return [self.internalValue hash];
313-
}
314-
315-
- (NSComparisonResult)compare:(FSTFieldValue *)other {
316-
if (other.type == FieldValue::Type::Blob) {
317-
return CompareBytes(self.internalValue, ((FSTBlobValue *)other).internalValue);
318-
} else {
319-
return [self defaultCompare:other];
320-
}
321-
}
322-
323-
@end
324-
325258
#pragma mark - FSTReferenceValue
326259

327260
@interface FSTReferenceValue ()
@@ -749,6 +682,7 @@ - (id)value {
749682
case FieldValue::Type::String:
750683
return util::WrapNSString(self.internalValue.string_value());
751684
case FieldValue::Type::Blob:
685+
return MakeNSData(self.internalValue.blob_value());
752686
case FieldValue::Type::Reference:
753687
HARD_FAIL("TODO(rsgowman): implement");
754688
case FieldValue::Type::GeoPoint: {

‎Firestore/Source/Remote/FSTSerializerBeta.mm‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
#include "Firestore/core/src/firebase/firestore/model/precondition.h"
5353
#include "Firestore/core/src/firebase/firestore/model/resource_path.h"
5454
#include "Firestore/core/src/firebase/firestore/model/transform_operations.h"
55+
#include "Firestore/core/src/firebase/firestore/nanopb/nanopb_util.h"
5556
#include "Firestore/core/src/firebase/firestore/remote/existence_filter.h"
5657
#include "Firestore/core/src/firebase/firestore/remote/watch_change.h"
5758
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
@@ -80,6 +81,7 @@
8081
using firebase::firestore::model::SnapshotVersion;
8182
using firebase::firestore::model::TargetId;
8283
using firebase::firestore::model::TransformOperation;
84+
using firebase::firestore::nanopb::MakeByteString;
8385
using firebase::firestore::remote::DocumentWatchChange;
8486
using firebase::firestore::remote::ExistenceFilter;
8587
using firebase::firestore::remote::ExistenceFilterWatchChange;
@@ -271,7 +273,7 @@ - (FSTFieldValue *)decodedFieldValue:(GCFSValue *)valueProto {
271273
return FieldValue::FromGeoPoint([self decodedGeoPoint:valueProto.geoPointValue]).Wrap();
272274

273275
case GCFSValue_ValueType_OneOfCase_BytesValue:
274-
return [FSTBlobValue blobValue:valueProto.bytesValue];
276+
return FieldValue::FromBlob(MakeByteString(valueProto.bytesValue)).Wrap();
275277

276278
case GCFSValue_ValueType_OneOfCase_ReferenceValue:
277279
return [self decodedReferenceValue:valueProto.referenceValue];

‎Firestore/core/src/firebase/firestore/api/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,5 +50,6 @@ cc_library(
5050
DEPENDS
5151
absl_meta
5252
firebase_firestore_api_input_validation
53+
firebase_firestore_core
5354
firebase_firestore_util
5455
)

‎Firestore/core/src/firebase/firestore/auth/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ cc_library(
3232
firebase_credentials_provider_apple.h
3333
firebase_credentials_provider_apple.mm
3434
DEPENDS
35+
FirebaseAuthInterop
3536
FirebaseCore
3637
GoogleUtilities
3738
firebase_firestore_auth_base

‎Firestore/core/src/firebase/firestore/local/local_serializer.cc‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include "Firestore/core/src/firebase/firestore/model/no_document.h"
3030
#include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
3131
#include "Firestore/core/src/firebase/firestore/model/unknown_document.h"
32+
#include "Firestore/core/src/firebase/firestore/nanopb/byte_string.h"
3233
#include "Firestore/core/src/firebase/firestore/nanopb/nanopb_util.h"
3334
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
3435
#include "Firestore/core/src/firebase/firestore/util/string_format.h"
@@ -45,6 +46,7 @@ using model::MutationBatch;
4546
using model::NoDocument;
4647
using model::SnapshotVersion;
4748
using model::UnknownDocument;
49+
using nanopb::ByteString;
4850
using nanopb::CheckedSize;
4951
using nanopb::Reader;
5052
using nanopb::Writer;
@@ -198,7 +200,9 @@ firestore_client_Target LocalSerializer::EncodeQueryData(
198200
result.last_listen_sequence_number = query_data.sequence_number();
199201
result.snapshot_version = rpc_serializer_.EncodeTimestamp(
200202
query_data.snapshot_version().timestamp());
201-
result.resume_token = rpc_serializer_.EncodeBytes(query_data.resume_token());
203+
204+
// Force a copy because pb_release would otherwise double-free.
205+
result.resume_token = nanopb::CopyBytesArray(query_data.resume_token().get());
202206

203207
const Query& query = query_data.query();
204208
if (query.IsDocumentQuery()) {
@@ -228,8 +232,7 @@ QueryData LocalSerializer::DecodeQueryData(
228232
proto.last_listen_sequence_number);
229233
SnapshotVersion version =
230234
rpc_serializer_.DecodeSnapshotVersion(reader, proto.snapshot_version);
231-
std::vector<uint8_t> resume_token =
232-
rpc_serializer_.DecodeBytes(proto.resume_token);
235+
ByteString resume_token(proto.resume_token);
233236
Query query = Query::Invalid();
234237

235238
switch (proto.which_target_type) {

0 commit comments

Comments
 (0)