diff --git a/objectivec/GPBMessage.h b/objectivec/GPBMessage.h index e9889925ac..90d6991de2 100644 --- a/objectivec/GPBMessage.h +++ b/objectivec/GPBMessage.h @@ -507,13 +507,25 @@ CF_EXTERN_C_END * Merges in the data from an `GPBUnknownFields`, meaning the data from the unknown fields gets * re-parsed so any known fields will be propertly set. * - * If the intent is to replace the message's unknown fields, call `-clearUnknownFields` first. + * If the intent is to *replace* the message's unknown fields, call `-clearUnknownFields` first. + * + * Since the data from the GPBUnknownFields will always be well formed, this call will almost never + * fail. What could cause it to fail is if the GPBUnknownFields contains a field values it is + * and error for the message's schema - i.e.: if it contains a length delimited field where the + * field number for the message is defined to be a _string_ field, however the length delimited + * data provide is not a valid UTF8 string. * * @param unknownFields The unknown fields to merge the data from. * @param extensionRegistry The extension registry to use to look up extensions, can be `nil`. + * @param errorPtr An optional error pointer to fill in with a failure + * reason if the data can not be parsed. Will only be + * filled in if the data failed to be parsed. + * + * @return Boolean indicating success. errorPtr will only be fill in on failure. **/ -- (void)mergeUnknownFields:(GPBUnknownFields *)unknownFields - extensionRegistry:(nullable id)extensionRegistry; +- (BOOL)mergeUnknownFields:(GPBUnknownFields *)unknownFields + extensionRegistry:(nullable id)extensionRegistry + error:(NSError **)errorPtr; @end diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m index 652c791ef0..384bf5b0d8 100644 --- a/objectivec/GPBMessage.m +++ b/objectivec/GPBMessage.m @@ -1484,14 +1484,12 @@ static void MergeUnknownFieldDataIntoFieldSet(GPBMessage *self, NSData *data, self.unknownFields = nil; } -- (void)mergeUnknownFields:(GPBUnknownFields *)unknownFields - extensionRegistry:(nullable id)extensionRegistry { - NSData *data = [unknownFields serializeAsData]; - if (![self mergeFromData:data extensionRegistry:extensionRegistry error:NULL]) { -#if defined(DEBUG) && DEBUG - NSAssert(0, @"Internal error within the library, failed to parse data from unknown fields."); -#endif - }; +- (BOOL)mergeUnknownFields:(GPBUnknownFields *)unknownFields + extensionRegistry:(nullable id)extensionRegistry + error:(NSError **)errorPtr { + return [self mergeFromData:[unknownFields serializeAsData] + extensionRegistry:extensionRegistry + error:errorPtr]; } - (BOOL)isInitialized { diff --git a/objectivec/GPBUnknownFields.h b/objectivec/GPBUnknownFields.h index 4f05c2b972..07a7af5f51 100644 --- a/objectivec/GPBUnknownFields.h +++ b/objectivec/GPBUnknownFields.h @@ -33,7 +33,7 @@ __attribute__((objc_subclassing_restricted)) * * Note: The instance is not linked to the message, any change will not be * reflected on the message the changes have to be pushed back to the message - * with `-[GPBMessage mergeUnknownFields:error:]`. + * with `-[GPBMessage mergeUnknownFields:extensionRegistry:error:]`. **/ - (instancetype)initFromMessage:(nonnull GPBMessage *)message; diff --git a/objectivec/Tests/GPBMessageTests.m b/objectivec/Tests/GPBMessageTests.m index f3272dbaf2..ab51091564 100644 --- a/objectivec/Tests/GPBMessageTests.m +++ b/objectivec/Tests/GPBMessageTests.m @@ -506,7 +506,7 @@ GPBUnknownFields *ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1234 fixed32:1234]; [ufs addFieldNumber:2345 varint:54321]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); NSString *description = [message description]; XCTAssertGreaterThan([description length], 0U); @@ -997,13 +997,17 @@ XCTAssertFalse([message hasOptionalNestedMessage]); GPBUnknownFields *ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1 varint:1]; - [message.optionalNestedMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message.optionalNestedMessage mergeUnknownFields:ufs + extensionRegistry:nil + error:NULL]); XCTAssertTrue([message hasOptionalNestedMessage]); message.optionalNestedMessage = nil; XCTAssertFalse([message hasOptionalNestedMessage]); [ufs clear]; // Also make sure merging zero length forces it to become visible. - [message.optionalNestedMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message.optionalNestedMessage mergeUnknownFields:ufs + extensionRegistry:nil + error:NULL]); XCTAssertTrue([message hasOptionalNestedMessage]); } @@ -1887,7 +1891,7 @@ [message setUnknownFields:unknowns]; GPBUnknownFields *ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1234 varint:5678]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); NSData *data = [message data]; GPBMessage *message2 = [GPBMessage parseFromData:data extensionRegistry:nil error:NULL]; XCTAssertEqualObjects(message, message2); @@ -1900,7 +1904,7 @@ [message1 setUnknownFields:unknowns1]; GPBUnknownFields *ufs1 = [[[GPBUnknownFields alloc] init] autorelease]; [ufs1 addFieldNumber:1234 varint:5678]; - [message1 mergeUnknownFields:ufs1 extensionRegistry:nil]; + XCTAssertTrue([message1 mergeUnknownFields:ufs1 extensionRegistry:nil error:NULL]); GPBUnknownFieldSet *unknowns2 = [[[GPBUnknownFieldSet alloc] init] autorelease]; [unknowns2 mergeVarintField:789 value:987]; @@ -1910,7 +1914,7 @@ GPBUnknownFields *ufs2 = [[[GPBUnknownFields alloc] init] autorelease]; [ufs2 addFieldNumber:2345 fixed32:6789]; [ufs2 addFieldNumber:3456 fixed32:7890]; - [message2 mergeUnknownFields:ufs2 extensionRegistry:nil]; + XCTAssertTrue([message2 mergeUnknownFields:ufs2 extensionRegistry:nil error:NULL]); NSMutableData *delimitedData = [NSMutableData data]; [delimitedData appendData:[message1 delimitedData]]; diff --git a/objectivec/Tests/GPBUnknownFieldsTest.m b/objectivec/Tests/GPBUnknownFieldsTest.m index 6c6481d480..c1e5e10634 100644 --- a/objectivec/Tests/GPBUnknownFieldsTest.m +++ b/objectivec/Tests/GPBUnknownFieldsTest.m @@ -811,7 +811,7 @@ [group addFieldNumber:123456 varint:5432]; TestAllTypes* msg = [TestAllTypes message]; - [msg mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([msg mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); XCTAssertEqual(msg.optionalInt64, 100); XCTAssertEqual(msg.optionalFixed32, 200); XCTAssertEqual(msg.optionalFixed64, 300); @@ -829,7 +829,7 @@ XCTAssertEqual(varint, 5432); TestEmptyMessage* emptyMessage = [TestEmptyMessage message]; - [emptyMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([emptyMessage mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); GPBUnknownFields* ufs3 = [[[GPBUnknownFields alloc] initFromMessage:emptyMessage] autorelease]; XCTAssertEqualObjects(ufs3, ufs); // Round trip through an empty message got us same fields back. XCTAssertTrue(ufs3 != ufs); // But they are different objects. @@ -843,7 +843,7 @@ TestEmptyMessage* emptyMessage = [TestEmptyMessage parseFromData:allFieldsData error:NULL]; GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] initFromMessage:emptyMessage] autorelease]; TestAllTypes* allFields2 = [TestAllTypes message]; - [allFields2 mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([allFields2 mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); XCTAssertEqualObjects(allFields2, allFields); // Confirm that the they still all end up in unknowns when parsed into a message with extensions @@ -891,7 +891,7 @@ // unknown fields again. { TestAllTypes* msg = [TestAllTypes message]; - [msg mergeUnknownFields:ufsWrongTypes extensionRegistry:nil]; + XCTAssertTrue([msg mergeUnknownFields:ufsWrongTypes extensionRegistry:nil error:NULL]); GPBUnknownFields* ufs2 = [[[GPBUnknownFields alloc] initFromMessage:msg] autorelease]; XCTAssertFalse(ufs2.empty); XCTAssertEqualObjects(ufs2, ufsWrongTypes); // All back as unknown fields. @@ -901,19 +901,46 @@ // into unknown fields. { TestAllExtensions* msg = [TestAllExtensions message]; - [msg mergeUnknownFields:ufsWrongTypes extensionRegistry:[UnittestRoot extensionRegistry]]; + XCTAssertTrue([msg mergeUnknownFields:ufsWrongTypes + extensionRegistry:[UnittestRoot extensionRegistry] + error:NULL]); GPBUnknownFields* ufs2 = [[[GPBUnknownFields alloc] initFromMessage:msg] autorelease]; XCTAssertFalse(ufs2.empty); XCTAssertEqualObjects(ufs2, ufsWrongTypes); // All back as unknown fields. } } +- (void)testMergeFailures { + // Valid data, pushes to the string just fine. + { + GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; + [ufs addFieldNumber:TestAllTypes_FieldNumber_OptionalString + lengthDelimited:DataFromCStr("abc")]; + TestAllTypes* msg = [TestAllTypes message]; + NSError* error = nil; + XCTAssertTrue([msg mergeUnknownFields:ufs extensionRegistry:nil error:&error]); + XCTAssertNil(error); + XCTAssertEqualObjects(msg.optionalString, @"abc"); + } + + // Invalid UTF-8 causes a failure when pushed to the message. + { + GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; + [ufs addFieldNumber:TestAllTypes_FieldNumber_OptionalString + lengthDelimited:DataFromBytes(0xC2, 0xF2, 0x0, 0x0, 0x0)]; + TestAllTypes* msg = [TestAllTypes message]; + NSError* error = nil; + XCTAssertFalse([msg mergeUnknownFields:ufs extensionRegistry:nil error:&error]); + XCTAssertNotNil(error); + } +} + - (void)testLargeVarint { GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1 varint:0x7FFFFFFFFFFFFFFFL]; TestEmptyMessage* emptyMessage = [TestEmptyMessage message]; - [emptyMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([emptyMessage mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); GPBUnknownFields* ufsParsed = [[[GPBUnknownFields alloc] initFromMessage:emptyMessage] autorelease]; diff --git a/objectivec/Tests/GPBUtilitiesTests.m b/objectivec/Tests/GPBUtilitiesTests.m index fb0175ecca..6db22a3d37 100644 --- a/objectivec/Tests/GPBUtilitiesTests.m +++ b/objectivec/Tests/GPBUtilitiesTests.m @@ -188,7 +188,7 @@ [group addFieldNumber:3 fixed32:0x3]; [group addFieldNumber:2 fixed64:0x2]; TestEmptyMessage *message = [TestEmptyMessage message]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); NSString *expected = @"# --- Unknown fields ---\n" @"10: 1\n" @@ -255,7 +255,8 @@ static GPBUnknownFieldSet *UnknownFieldsSetHelper(int num) { static void AddUnknownFields(GPBMessage *message, int num) { GPBUnknownFields *ufs = [[GPBUnknownFields alloc] init]; [ufs addFieldNumber:num varint:num]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + // Can't fail since it is a varint. + [message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]; [ufs release]; } diff --git a/objectivec/Tests/GPBWireFormatTests.m b/objectivec/Tests/GPBWireFormatTests.m index 3d32112f17..2df9f2d6b6 100644 --- a/objectivec/Tests/GPBWireFormatTests.m +++ b/objectivec/Tests/GPBWireFormatTests.m @@ -131,7 +131,9 @@ const int kUnknownTypeId2 = 1550056; GPBUnknownFields* group = [ufs addGroupWithFieldNumber:GPBWireFormatMessageSetItem]; [group addFieldNumber:GPBWireFormatMessageSetTypeId varint:kUnknownTypeId2]; [group addFieldNumber:GPBWireFormatMessageSetMessage lengthDelimited:DataFromCStr("baz")]; - [message_set mergeUnknownFields:ufs extensionRegistry:[MSetUnittestMsetRoot extensionRegistry]]; + XCTAssertTrue([message_set mergeUnknownFields:ufs + extensionRegistry:[MSetUnittestMsetRoot extensionRegistry] + error:NULL]); NSData* data = [message_set data];