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.

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

Add partial support to binary serialization support. #303

Merged
merged 1 commit into from
Feb 23, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions Sources/SwiftProtobuf/BinaryEncodingError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,8 @@ public enum BinaryEncodingError: Error {
/// binary unless the object they hold is a well-known type or a
/// type registered with via Google_Protobuf_Any.register()
case anyTranscodeFailure
/// The message or nested messages definitions have required fields, and the
/// Message being encoded does not include values for some of them. The
/// `partial` support will allow this incomplete data to be decoded.
case missingRequiredFields
}
4 changes: 3 additions & 1 deletion Sources/SwiftProtobuf/BinaryEncodingVisitor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ internal struct BinaryEncodingVisitor: Visitor {

mutating func visitSingularMessageField<M: Message>(value: M,
fieldNumber: Int) throws {
let t = try value.serializedData()
// Can force partial to true here because the parent message would have
// already recursed for the isInitialized check if it was needed.
let t = try value.serializedData(partial: true)
encoder.startField(fieldNumber: fieldNumber, wireFormat: .lengthDelimited)
encoder.putBytesValue(value: t)
}
Expand Down
16 changes: 15 additions & 1 deletion Sources/SwiftProtobuf/BinaryTypeAdditions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,18 @@ import Foundation
/// Messages
///
public extension Message {
func serializedData() throws -> Data {
/// Serializes the message to the Protocol Buffer binary serialization format.
///
/// - Parameters:
/// - partial: The binary serialization format requires all `required` fields
/// be present; when `partial` is `false`, `EncodingError.missingRequiredFields`
/// is throw if any were missing. When `partial` is `true`, then partial
/// messages are allowed, and `Message.isRequired` is not checked.
/// - Throws: An instance of `EncodingError` on failure .
func serializedData(partial: Bool = false) throws -> Data {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be different for proto2 vs. proto3? (Since proto3 doesn't have required and never will.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

A proto3 message could still have a proto2 message nested inside it, so partial needs to be supported there as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah. Good point.

if !partial && !isInitialized {
throw BinaryEncodingError.missingRequiredFields
}
let requiredSize = try serializedDataSize()
var data = Data(count: requiredSize)
try data.withUnsafeMutableBytes { (pointer: UnsafeMutablePointer<UInt8>) in
Expand All @@ -34,6 +45,9 @@ public extension Message {
}

internal func serializedDataSize() throws -> Int {
// Note: since this api is internal, it doesn't currently worry about
// needing a partial argument to handle proto2 syntax required fields.
// If this become public, it will need that added.
var visitor = BinaryEncodingSizeVisitor()
try traverse(visitor: &visitor)
return visitor.serializedSize
Expand Down
6 changes: 4 additions & 2 deletions Tests/LinuxMain.swift
Original file line number Diff line number Diff line change
Expand Up @@ -786,15 +786,17 @@ extension Test_Required {
("test_NestedInProto3_IsInitialized", {try run_test(test:($0 as! Test_Required).test_NestedInProto3_IsInitialized)}),
("test_map_isInitialized", {try run_test(test:($0 as! Test_Required).test_map_isInitialized)}),
("test_Extensions_isInitialized", {try run_test(test:($0 as! Test_Required).test_Extensions_isInitialized)}),
("test_decodeRequired", {try run_test(test:($0 as! Test_Required).test_decodeRequired)})
("test_decodeRequired", {try run_test(test:($0 as! Test_Required).test_decodeRequired)}),
("test_encodeRequired", {try run_test(test:($0 as! Test_Required).test_encodeRequired)})
]
}
}

extension Test_SmallRequired {
static var allTests: [(String, (XCTestCase) throws -> ())] {
return [
("test_decodeRequired", {try run_test(test:($0 as! Test_SmallRequired).test_decodeRequired)})
("test_decodeRequired", {try run_test(test:($0 as! Test_SmallRequired).test_decodeRequired)}),
("test_encodeRequired", {try run_test(test:($0 as! Test_SmallRequired).test_encodeRequired)})
]
}
}
Expand Down
162 changes: 159 additions & 3 deletions Tests/SwiftProtobufTests/Test_Required.swift
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ class Test_Required: XCTestCase, PBTestHelpers {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed this: We should have a real text format serializer method in addition to the debugDescription hook (because people actually do use text format as an interchange format). That would remove the need to play games just above to massage the expected text into the debugDescription format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do (https://github.com/apple/swift-protobuf/blob/master/Sources/SwiftProtobuf/TextFormatTypeAdditions.swift#L22), it still throws, which have an issue to clean up. So for the moment I believe the debugDescription is a hair easier to deal with. Once it does't throw, we're probably good to use it in testing asserts.


func test_decodeRequired() throws {
// Empty empty
// Empty message.
assertDecodeFailsNotInitialized([])
assertPartialDecodeSucceeds([], "")

Expand All @@ -186,7 +186,7 @@ class Test_Required: XCTestCase, PBTestHelpers {
([93, 0, 0, 48, 65], "required_float: 11"),
([97, 0, 0, 0, 0, 0, 0, 40, 64], "required_double: 12"),
([104, 1], "required_bool: true"),
([114, 2, 49, 51], "required_string: \"13\""),
([114, 2, 49, 52], "required_string: \"14\""),
([122, 1, 15], "required_bytes: \"\\017\""),
([131, 1, 136, 1, 16, 132, 1], "RequiredGroup {\n a: 16\n}"),
([146, 1, 2, 8, 18], "required_nested_message {\n bb: 18\n}"),
Expand Down Expand Up @@ -236,6 +236,104 @@ class Test_Required: XCTestCase, PBTestHelpers {
let fullMsg = try ProtobufUnittest_TestAllRequiredTypes(serializedData: allBytesData)
XCTAssertEqual(fullMsg.debugDescription, allTextFormattedField)
}

// Helper to assert encoding fails with a not initialized error.
fileprivate func assertEncodeFailsNotInitialized(_ message: MessageTestType, file: XCTestFileArgType = #file, line: UInt = #line) {
do {
let _ = try message.serializedData()
XCTFail("Swift encode should have failed: \(message)", file: file, line: line)
} catch BinaryEncodingError.missingRequiredFields {
// Correct error!
} catch let e {
XCTFail("Encoding got wrong error: \(e) for \(message)", file: file, line: line)
}
}

// Helper to assert encoding partial succeeds.
fileprivate func assertPartialEncodeSucceeds(_ message: MessageTestType, _ expectedBytes: [UInt8], file: XCTestFileArgType = #file, line: UInt = #line) {
do {
let data = try message.serializedData(partial: true)
XCTAssertEqual(data, Data(bytes:expectedBytes), "While encoding \(message)", file: file, line: line)
} catch let e {
XCTFail("Encoding failed with error: \(e) for \(message)", file: file, line: line)
}
}

func test_encodeRequired() throws {
var msg = MessageTestType()

// Empty message.
assertEncodeFailsNotInitialized(msg)
assertPartialEncodeSucceeds(msg, [])

typealias ConfigurationBlock = (inout MessageTestType) -> Void

// Test every field on its own.
let testInputs: [([UInt8], ConfigurationBlock)] = [
([8, 1], { (m) in m.requiredInt32 = 1 }),
([16, 2], { (m) in m.requiredInt64 = 2 }),
([24, 3], { (m) in m.requiredUint32 = 3 }),
([32, 4], { (m) in m.requiredUint64 = 4 }),
([40, 10], { (m) in m.requiredSint32 = 5 }),
([48, 12], { (m) in m.requiredSint64 = 6 }),
([61, 7, 0, 0, 0], { (m) in m.requiredFixed32 = 7 }),
([65, 8, 0, 0, 0, 0, 0, 0, 0], { (m) in m.requiredFixed64 = 8 }),
([77, 9, 0, 0, 0], { (m) in m.requiredSfixed32 = 9 }),
([81, 10, 0, 0, 0, 0, 0, 0, 0], { (m) in m.requiredSfixed64 = 10 }),
([93, 0, 0, 48, 65], { (m) in m.requiredFloat = 11 }),
([97, 0, 0, 0, 0, 0, 0, 40, 64], { (m) in m.requiredDouble = 12 }),
([104, 1], { (m) in m.requiredBool = true }),
([114, 2, 49, 52], { (m) in m.requiredString = "14" }),
([122, 1, 15], { (m) in m.requiredBytes = Data(bytes: [15]) }),
([131, 1, 136, 1, 16, 132, 1], { (m) in m.requiredGroup.a = 16 }),
([146, 1, 2, 8, 18], { (m) in m.requiredNestedMessage.bb = 18 }),
([154, 1, 2, 8, 19], { (m) in m.requiredForeignMessage.c = 19 }),
([162, 1, 2, 8, 20], { (m) in m.requiredImportMessage.d = 20 }),
([168, 1, 3], { (m) in m.requiredNestedEnum = .baz }),
([176, 1, 5], { (m) in m.requiredForeignEnum = .foreignBar }),
([184, 1, 9], { (m) in m.requiredImportEnum = .importBaz }),
([194, 1, 2, 50, 52], { (m) in m.requiredStringPiece = "24" }),
([202, 1, 2, 50, 53], { (m) in m.requiredCord = "25" }),
([210, 1, 2, 8, 26], { (m) in m.requiredPublicImportMessage.e = 26 }),
([218, 1, 2, 8, 27], { (m) in m.requiredLazyMessage.bb = 27 }),
([232, 3, 61], { (m) in m.defaultInt32 = 61 }),
([240, 3, 62], { (m) in m.defaultInt64 = 62 }),
([248, 3, 63], { (m) in m.defaultUint32 = 63 }),
([128, 4, 64], { (m) in m.defaultUint64 = 64 }),
([136, 4, 130, 1], { (m) in m.defaultSint32 = 65 }),
([144, 4, 132, 1], { (m) in m.defaultSint64 = 66 }),
([157, 4, 67, 0, 0, 0], { (m) in m.defaultFixed32 = 67 }),
([161, 4, 68, 0, 0, 0, 0, 0, 0, 0], { (m) in m.defaultFixed64 = 68 }),
([173, 4, 69, 0, 0, 0], { (m) in m.defaultSfixed32 = 69 }),
([177, 4, 70, 0, 0, 0, 0, 0, 0, 0], { (m) in m.defaultSfixed64 = 70 }),
([189, 4, 0, 0, 142, 66], { (m) in m.defaultFloat = 71 }),
([193, 4, 0, 0, 0, 0, 0, 0, 82, 64], { (m) in m.defaultDouble = 72 }),
([200, 4, 0], { (m) in m.defaultBool = false }),
([210, 4, 2, 55, 52], { (m) in m.defaultString = "74" }),
([218, 4, 1, 75], { (m) in m.defaultBytes = Data(bytes: [75]) }),
([136, 5, 3], { (m) in m.defaultNestedEnum = .baz }),
([144, 5, 6], { (m) in m.defaultForeignEnum = .foreignBaz }),
([152, 5, 9], { (m) in m.defaultImportEnum = .importBaz }),
([162, 5, 2, 56, 52], { (m) in m.defaultStringPiece = "84" }),
([170, 5, 2, 56, 53], { (m) in m.defaultCord = "85" }),
]
for (expected, configure) in testInputs {
var message = MessageTestType()
configure(&message)
assertEncodeFailsNotInitialized(message)
assertPartialEncodeSucceeds(message, expected)
}

// Glue it all together and it should encode ok as it will be complete.
var allExpectedData = Data()
msg = MessageTestType()
for (expected, configure) in testInputs {
allExpectedData.append(Data(bytes:expected))
configure(&msg)
}
let serialized = try msg.serializedData()
XCTAssertEqual(serialized, allExpectedData)
}
}

class Test_SmallRequired: XCTestCase, PBTestHelpers {
Expand Down Expand Up @@ -269,7 +367,7 @@ class Test_SmallRequired: XCTestCase, PBTestHelpers {
}

func test_decodeRequired() throws {
// Empty empty
// Empty message.
assertDecodeFailsNotInitialized([])
assertPartialDecodeSucceeds([], "")

Expand Down Expand Up @@ -298,4 +396,62 @@ class Test_SmallRequired: XCTestCase, PBTestHelpers {
let fullMsg = try ProtobufUnittest_TestSomeRequiredTypes(serializedData: allBytesData)
XCTAssertEqual(fullMsg.debugDescription, allTextFormattedField)
}

// Helper to assert encoding fails with a not initialized error.
fileprivate func assertEncodeFailsNotInitialized(_ message: MessageTestType, file: XCTestFileArgType = #file, line: UInt = #line) {
do {
let _ = try message.serializedData()
XCTFail("Swift encode should have failed: \(message)", file: file, line: line)
} catch BinaryEncodingError.missingRequiredFields {
// Correct error!
} catch let e {
XCTFail("Encoding got wrong error: \(e) for \(message)", file: file, line: line)
}
}

// Helper to assert encoding partial succeeds.
fileprivate func assertPartialEncodeSucceeds(_ message: MessageTestType, _ expectedBytes: [UInt8], file: XCTestFileArgType = #file, line: UInt = #line) {
do {
let data = try message.serializedData(partial: true)
XCTAssertEqual(data, Data(bytes:expectedBytes), "While encoding \(message)", file: file, line: line)
} catch let e {
XCTFail("Encoding failed with error: \(e) for \(message)", file: file, line: line)
}
}

func test_encodeRequired() throws {
var msg = MessageTestType()

// Empty message.
assertEncodeFailsNotInitialized(msg)
assertPartialEncodeSucceeds(msg, [])

typealias ConfigurationBlock = (inout MessageTestType) -> Void

// Test every field on its own.
let testInputs: [([UInt8], ConfigurationBlock)] = [
([8, 1], { (m) in m.requiredInt32 = 1 }),
([21, 0, 0, 0, 64], { (m) in m.requiredFloat = 2 }),
([24, 1], { (m) in m.requiredBool = true }),
([34, 1, 52], { (m) in m.requiredString = "4" }),
([42, 1, 5], { (m) in m.requiredBytes = Data(bytes: [5]) }),
([48, 1], { (m) in m.requiredNestedEnum = .foo }),
]
for (expected, configure) in testInputs {
var message = MessageTestType()
configure(&message)
assertEncodeFailsNotInitialized(message)
assertPartialEncodeSucceeds(message, expected)
}

// Glue it all together and it should encode ok as it will be complete.
var allExpectedData = Data()
msg = MessageTestType()
for (expected, configure) in testInputs {
allExpectedData.append(Data(bytes:expected))
configure(&msg)
}
let serialized = try msg.serializedData()
XCTAssertEqual(serialized, allExpectedData)
}
}