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

Fix oneof primitive types decoding #1632

Closed

Conversation

pouyayarandi
Copy link

This PR Fixes #1608

import XCTest
import SwiftProtobuf

final class Test_OneofFields_Decoding: XCTestCase {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's already Test_Merge and Test_ParsingMerge, so I'd suggest extending one of those (maybe they should merge).

We also want to make sure everything is covered

  • non repeating fields of pod types override
  • non repeating fields of message types recursively merge
  • repeating fields append
  • oneof fields for pod overrides to the new value
  • oneof fields for a message merges
  • there's likely cases with extension fields also (repeating/non repeating, pod/message)

I also don't think you need the custom decoder here, just create a two messages, and then use the merge api on the binary data from the second message. There's some examples in the other files.

@@ -345,6 +345,7 @@ class OneofGenerator {
hadValueTest = "hadOneofValue"
} else {
p.print("var v: \(field.swiftType)?")
p.print("if case \(field.dottedSwiftName)(let _v) = \(storedProperty) {v = _v}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you split this change out, since this is the actual generator change, it makes it much easier to see.

@@ -491,6 +491,7 @@ extension Conformance_ConformanceRequest: SwiftProtobuf.Message, SwiftProtobuf._
switch fieldNumber {
case 1: try {
var v: Data?
if case .protobufPayload(let _v) = self.payload {v = _v}
try decoder.decodeSingularBytesField(value: &v)
if let v = v {
if self.payload != nil {try decoder.handleConflictingOneOf()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part I'm also no longer sure of.

If the oneof was already set to the same failed, is it always a potentially conflicting oneof? It likely is up to the decoder, but there is no longer enough info to say if it was already set to the same value or was set to a different oneof value.

@pouyayarandi
Copy link
Author

I checked Test_Merge and realized that it works with oneof fields. So I think the problem is my own code (which I was using decoder for capturing values in #1505).

I think we can close this PR and its related issue as my problem solved with changing my code implementation and without any necessary addition in generated codes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Primitive one-of type always passes nil as its argument for decoder
2 participants