{"action":"closed","issue":{"url":"https://api.github.com/repos/alexeyxo/protobuf-swift/issues/179","repository_url":"https://api.github.com/repos/alexeyxo/protobuf-swift","labels_url":"https://api.github.com/repos/alexeyxo/protobuf-swift/issues/179/labels{/name}","comments_url":"https://api.github.com/repos/alexeyxo/protobuf-swift/issues/179/comments","events_url":"https://api.github.com/repos/alexeyxo/protobuf-swift/issues/179/events","html_url":"https://github.com/alexeyxo/protobuf-swift/issues/179","id":177241883,"number":179,"title":"Problem with handling implied packing in Proto3","user":{"login":"gmckenzi","id":242154,"avatar_url":"https://avatars.githubusercontent.com/u/242154?v=3","gravatar_id":"","url":"https://api.github.com/users/gmckenzi","html_url":"https://github.com/gmckenzi","followers_url":"https://api.github.com/users/gmckenzi/followers","following_url":"https://api.github.com/users/gmckenzi/following{/other_user}","gists_url":"https://api.github.com/users/gmckenzi/gists{/gist_id}","starred_url":"https://api.github.com/users/gmckenzi/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gmckenzi/subscriptions","organizations_url":"https://api.github.com/users/gmckenzi/orgs","repos_url":"https://api.github.com/users/gmckenzi/repos","events_url":"https://api.github.com/users/gmckenzi/events{/privacy}","received_events_url":"https://api.github.com/users/gmckenzi/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/alexeyxo/protobuf-swift/labels/bug","name":"bug","color":"fc2929"},{"url":"https://api.github.com/repos/alexeyxo/protobuf-swift/labels/high%20priority","name":"high priority","color":"eb6420"},{"url":"https://api.github.com/repos/alexeyxo/protobuf-swift/labels/swift3.0","name":"swift3.0","color":"fad8c7"}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-09-15T17:48:53Z","updated_at":"2016-09-16T20:55:45Z","closed_at":"2016-09-16T20:55:45Z","body":"#### Version of protoc (```protoc --version```)\r\n###### 3.0\r\n\r\n#### Version of ProtocolBuffers.framework\r\n###### 3.0.0\r\n\r\n#### ```.proto``` file to reproduce\r\n```protobuf\r\nsyntax = \"proto3\";\r\npackage Test;\r\n\r\nenum Instructions\r\n{\r\n    InstructionA = 0;\r\n    InstructionB  = 1;\r\n    InstructionC  = 2;\r\n}\r\n\r\nmessage Value \r\n{\r\n\tbool   b = 1;\r\n}\r\n\r\nmessage Expression\r\n{\r\n    repeated Instructions code\t= 1;\r\n\trepeated Value data\t\t\t= 2 [packed=false];\r\n}\r\n```\r\n\r\n#### Description\r\n\r\nMy understanding is that in Protobuf v3, packing is turned on by default. However, the Protobuf docs also add this bit of complication: \r\n\r\n> Protocol buffer parsers must be able to parse repeated fields that were compiled as packed as if they were not packed, and vice versa.\r\n\r\nSo, the above sample protobuf file is derived from a client project I'm working on. The Expression message contains a repeated Instructions enum, and a repeated Value message. The Value message is not relevant to this problem except to point out that in my client's protobuf definition they leave \"packed=true\" off the Instructions part because packing is implied in Protobuf v3, and therefore they explicitly say \"packed=false\" on the value part because for some reason they don't want the repeated values to be packed.\r\n\r\nWhen I compiled this .proto file to Swift, it generates code which apparently assumes a \"packed=false\" behaviour for the repeated Instructions enum. This is a problem because the client's server is packing the repeated Instructions enum.\r\n\r\nWhen I compile this .proto to Go and run the sample program, I can see that Go packs the Instructions by default.\r\n\r\n```go\r\npackage main\r\n\r\nimport \"log\"\r\nimport proto \"github.com/golang/protobuf/proto\"\r\nimport pb \"Test\"\r\n\r\nfunc main() {\r\n\texpr := &pb.Expression{Code: []pb.Instructions{pb.Instructions_InstructionA, pb.Instructions_InstructionB}}\r\n\tdata, _ := proto.Marshal(expr)\r\n\tlog.Printf(\"Go: %x\\n\", data)\r\n}\r\n```\r\n\r\nIt produces the following output:\r\n\r\n`Go: 0a020001`\r\n\r\nSo, the leading byte is 10, indicating the beginning of the packing.\r\n\r\nWhen I look at the Swift code generated, I see this section:\r\n\r\n```swift\r\n      override public func mergeFrom(codedInputStream: CodedInputStream, extensionRegistry:ExtensionRegistry) throws -> Test.Expression.Builder {\r\n        let unknownFieldsBuilder:UnknownFieldSet.Builder = try UnknownFieldSet.builderWithUnknownFields(copyFrom:self.unknownFields)\r\n        while (true) {\r\n          let protobufTag = try codedInputStream.readTag()\r\n          switch protobufTag {\r\n          case 0: \r\n            self.unknownFields = try unknownFieldsBuilder.build()\r\n            return self\r\n\r\n          case 8:\r\n            let valueIntcode = try codedInputStream.readEnum()\r\n            if let enumscode = Test.Instructions(rawValue:valueIntcode) {\r\n                 builderResult.code.append(enumscode)\r\n            } else {\r\n                 _ = try unknownFieldsBuilder.mergeVarintField(fieldNumber: 1, value:Int64(valueIntcode))\r\n            }\r\n```\r\n\r\nNote the `case 8:` instead of the expected `case 10`.\r\n\r\nIf I modify the .proto file to force packing on for the repeated Instructions enum...\r\n\r\n```protobuf\r\nsyntax = \"proto3\";\r\npackage Test;\r\n\r\nenum Instructions\r\n{\r\n    InstructionA = 0;\r\n    InstructionB  = 1;\r\n    InstructionC  = 2;\r\n}\r\n\r\nmessage Value \r\n{\r\n\tbool   b = 1;\r\n}\r\n\r\nmessage Expression\r\n{\r\n    repeated Instructions code\t= 1 [packed=true];\r\n\trepeated Value data\t\t\t= 2 [packed=false];\r\n}\r\n```\r\n\r\n... then the code is generated properly with a `case 10:` in the switch statement and code to handle the packing:\r\n\r\n```swift\r\n      override public func mergeFrom(codedInputStream: CodedInputStream, extensionRegistry:ExtensionRegistry) throws -> Test.Expression.Builder {\r\n        let unknownFieldsBuilder:UnknownFieldSet.Builder = try UnknownFieldSet.builderWithUnknownFields(copyFrom:self.unknownFields)\r\n        while (true) {\r\n          let protobufTag = try codedInputStream.readTag()\r\n          switch protobufTag {\r\n          case 0: \r\n            self.unknownFields = try unknownFieldsBuilder.build()\r\n            return self\r\n\r\n          case 10:\r\n            let length = Int(try codedInputStream.readRawVarint32())\r\n            let oldLimit = try codedInputStream.pushLimit(byteLimit: length)\r\n            while codedInputStream.bytesUntilLimit() > 0 {\r\n            let valueIntcode = try codedInputStream.readEnum()\r\n            if let enumscode = Test.Instructions(rawValue:valueIntcode) {\r\n                 builderResult.code.append(enumscode)\r\n            } else {\r\n                 _ = try unknownFieldsBuilder.mergeVarintField(fieldNumber: 1, value:Int64(valueIntcode))\r\n            }\r\n            }\r\n            codedInputStream.popLimit(oldLimit: oldLimit)\r\n```\r\n\r\nSo, my problem is that my client's server is treating the Protobuf v3 files as if packing is implied on eligible (primitive type) repeating items, but protobuf-swift is treating it as if packing is implied off and generating code that breaks when it encounters the packed data, interpreting it as unrecognized bytes.\r\n\r\nIMHO the bug here is that protobuf-swift needs to assume `packed=true` when processing repeated primitive types in Protobuf v3 files.\r\n\r\nComing back to Google's statement...\r\n\r\n> Protocol buffer parsers must be able to parse repeated fields that were compiled as packed as if they were not packed, and vice versa.\r\n\r\n... it would seem that having to respect this statement in the docs this would seriously complicate the code generator, as it would have to detect and handle both scenarios regardless of whether packing is explicitly declared as on or off. I'd be happy if protobuf-swift simply assumed packed=true where applicable when processing Protobuf v3 files."}}
{"actor":{"display_login":"alexeyxo"}}