Record zero for "has" for proto3 if in a oneof.

If a message is proto3, then the zero values still count as being set one the field is in a oneof. Add tests to confirm oneofs work as expected in both syntaxes.
9 years ago · a877fdfafe
parent 116596a4e6
commit a877fdfafe
3 changed files with 296 additions and 29 deletions
--- a/conformance/failure_list_objc.txt
+++ b/conformance/failure_list_objc.txt
@ -1,4 +1,4 @@
-# No tests currently failing.
+# All tests currently passing.
 #
-# json input or output tests are skipped (in conformance_objc.m) as mobile
-# platforms don't support json wire format to avoid code bloat.
+# JSON input or output tests are skipped (in conformance_objc.m) as mobile
+# platforms don't support JSON wire format to avoid code bloat.
--- a/objectivec/GPBUtilities.m
+++ b/objectivec/GPBUtilities.m
@ -218,9 +218,10 @@ void GPBMaybeClearOneof(GPBMessage *self, GPBOneofDescriptor *oneof,
 //%  TYPE *typePtr = (TYPE *)&storage[field->description_->offset];
 //%  *typePtr = value;
 //%  // proto2: any value counts as having been set; proto3, it
-//%  // has to be a non zero value.
-//%  BOOL hasValue =
-//%    (syntax == GPBFileSyntaxProto2) || (value != (TYPE)0);
+//%  // has to be a non zero value or be in a oneof.
+//%  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+//%                   || (value != (TYPE)0)
+//%                   || (field->containingOneof_ != NULL));
 //%  GPBSetHasIvarField(self, field, hasValue);
 //%  GPBBecomeVisibleToAutocreator(self);
 //%}
@ -337,8 +338,19 @@ void GPBSetRetainedObjectIvarWithFieldInternal(GPBMessage *self,
    // zero, they are being cleared.
    if ((syntax == GPBFileSyntaxProto3) && !fieldIsMessage &&
        ([value length] == 0)) {
-      setHasValue = NO;
-      value = nil;
+      // Except, if the field was in a oneof, then it still gets recorded as
+      // having been set so the state of the oneof can be serialized back out.
+      if (!oneof) {
+        setHasValue = NO;
+      }
+      if (setHasValue) {
+        NSCAssert(value != nil, @"Should never be setting has for nil");
+      } else {
+        // The value passed in was retained, it must be released since we
+        // aren't saving anything in the field.
+        [value release];
+        value = nil;
+      }
    }
    GPBSetHasIvarField(self, field, setHasValue);
  }
@ -524,9 +536,10 @@ void GPBSetBoolIvarWithFieldInternal(GPBMessage *self,
  GPBSetHasIvar(self, (int32_t)(fieldDesc->offset), fieldDesc->number, value);

  // proto2: any value counts as having been set; proto3, it
-  // has to be a non zero value.
-  BOOL hasValue =
-    (syntax == GPBFileSyntaxProto2) || (value != (BOOL)0);
+  // has to be a non zero value or be in a oneof.
+  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+                   || (value != (BOOL)0)
+                   || (field->containingOneof_ != NULL));
  GPBSetHasIvarField(self, field, hasValue);
  GPBBecomeVisibleToAutocreator(self);
 }
@ -573,9 +586,10 @@ void GPBSetInt32IvarWithFieldInternal(GPBMessage *self,
  int32_t *typePtr = (int32_t *)&storage[field->description_->offset];
  *typePtr = value;
  // proto2: any value counts as having been set; proto3, it
-  // has to be a non zero value.
-  BOOL hasValue =
-    (syntax == GPBFileSyntaxProto2) || (value != (int32_t)0);
+  // has to be a non zero value or be in a oneof.
+  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+                   || (value != (int32_t)0)
+                   || (field->containingOneof_ != NULL));
  GPBSetHasIvarField(self, field, hasValue);
  GPBBecomeVisibleToAutocreator(self);
 }
@ -622,9 +636,10 @@ void GPBSetUInt32IvarWithFieldInternal(GPBMessage *self,
  uint32_t *typePtr = (uint32_t *)&storage[field->description_->offset];
  *typePtr = value;
  // proto2: any value counts as having been set; proto3, it
-  // has to be a non zero value.
-  BOOL hasValue =
-    (syntax == GPBFileSyntaxProto2) || (value != (uint32_t)0);
+  // has to be a non zero value or be in a oneof.
+  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+                   || (value != (uint32_t)0)
+                   || (field->containingOneof_ != NULL));
  GPBSetHasIvarField(self, field, hasValue);
  GPBBecomeVisibleToAutocreator(self);
 }
@ -671,9 +686,10 @@ void GPBSetInt64IvarWithFieldInternal(GPBMessage *self,
  int64_t *typePtr = (int64_t *)&storage[field->description_->offset];
  *typePtr = value;
  // proto2: any value counts as having been set; proto3, it
-  // has to be a non zero value.
-  BOOL hasValue =
-    (syntax == GPBFileSyntaxProto2) || (value != (int64_t)0);
+  // has to be a non zero value or be in a oneof.
+  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+                   || (value != (int64_t)0)
+                   || (field->containingOneof_ != NULL));
  GPBSetHasIvarField(self, field, hasValue);
  GPBBecomeVisibleToAutocreator(self);
 }
@ -720,9 +736,10 @@ void GPBSetUInt64IvarWithFieldInternal(GPBMessage *self,
  uint64_t *typePtr = (uint64_t *)&storage[field->description_->offset];
  *typePtr = value;
  // proto2: any value counts as having been set; proto3, it
-  // has to be a non zero value.
-  BOOL hasValue =
-    (syntax == GPBFileSyntaxProto2) || (value != (uint64_t)0);
+  // has to be a non zero value or be in a oneof.
+  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+                   || (value != (uint64_t)0)
+                   || (field->containingOneof_ != NULL));
  GPBSetHasIvarField(self, field, hasValue);
  GPBBecomeVisibleToAutocreator(self);
 }
@ -769,9 +786,10 @@ void GPBSetFloatIvarWithFieldInternal(GPBMessage *self,
  float *typePtr = (float *)&storage[field->description_->offset];
  *typePtr = value;
  // proto2: any value counts as having been set; proto3, it
-  // has to be a non zero value.
-  BOOL hasValue =
-    (syntax == GPBFileSyntaxProto2) || (value != (float)0);
+  // has to be a non zero value or be in a oneof.
+  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+                   || (value != (float)0)
+                   || (field->containingOneof_ != NULL));
  GPBSetHasIvarField(self, field, hasValue);
  GPBBecomeVisibleToAutocreator(self);
 }
@ -818,9 +836,10 @@ void GPBSetDoubleIvarWithFieldInternal(GPBMessage *self,
  double *typePtr = (double *)&storage[field->description_->offset];
  *typePtr = value;
  // proto2: any value counts as having been set; proto3, it
-  // has to be a non zero value.
-  BOOL hasValue =
-    (syntax == GPBFileSyntaxProto2) || (value != (double)0);
+  // has to be a non zero value or be in a oneof.
+  BOOL hasValue = ((syntax == GPBFileSyntaxProto2)
+                   || (value != (double)0)
+                   || (field->containingOneof_ != NULL));
  GPBSetHasIvarField(self, field, hasValue);
  GPBBecomeVisibleToAutocreator(self);
 }
--- a/objectivec/Tests/GPBMessageTests+Runtime.m
+++ b/objectivec/Tests/GPBMessageTests+Runtime.m
@ -1972,6 +1972,254 @@
  [msg release];
 }

+- (void)testProto2OneofSetToDefault {
+
+  // proto3 doesn't normally write out zero (default) fields, but if they are
+  // in a oneof it does.  proto2 doesn't have this special behavior, but we
+  // still confirm setting to the explicit default does set the case to be
+  // sure the runtime is working correctly.
+
+  NSString *oneofStringDefault = @"string";
+  NSData *oneofBytesDefault = [@"data" dataUsingEncoding:NSUTF8StringEncoding];
+
+  Message2 *msg = [[Message2 alloc] init];
+
+  uint32_t values[] = {
+    Message2_O_OneOfCase_OneofInt32,
+    Message2_O_OneOfCase_OneofInt64,
+    Message2_O_OneOfCase_OneofUint32,
+    Message2_O_OneOfCase_OneofUint64,
+    Message2_O_OneOfCase_OneofSint32,
+    Message2_O_OneOfCase_OneofSint64,
+    Message2_O_OneOfCase_OneofFixed32,
+    Message2_O_OneOfCase_OneofFixed64,
+    Message2_O_OneOfCase_OneofSfixed32,
+    Message2_O_OneOfCase_OneofSfixed64,
+    Message2_O_OneOfCase_OneofFloat,
+    Message2_O_OneOfCase_OneofDouble,
+    Message2_O_OneOfCase_OneofBool,
+    Message2_O_OneOfCase_OneofString,
+    Message2_O_OneOfCase_OneofBytes,
+    // Skip group
+    // Skip message
+    Message2_O_OneOfCase_OneofEnum,
+  };
+
+  for (size_t i = 0; i < GPBARRAYSIZE(values); ++i) {
+    switch (values[i]) {
+      case Message3_O_OneOfCase_OneofInt32:
+        msg.oneofInt32 = 100;
+        break;
+      case Message3_O_OneOfCase_OneofInt64:
+        msg.oneofInt64 = 101;
+        break;
+      case Message3_O_OneOfCase_OneofUint32:
+        msg.oneofUint32 = 102;
+        break;
+      case Message3_O_OneOfCase_OneofUint64:
+        msg.oneofUint64 = 103;
+        break;
+      case Message3_O_OneOfCase_OneofSint32:
+        msg.oneofSint32 = 104;
+        break;
+      case Message3_O_OneOfCase_OneofSint64:
+        msg.oneofSint64 = 105;
+        break;
+      case Message3_O_OneOfCase_OneofFixed32:
+        msg.oneofFixed32 = 106;
+        break;
+      case Message3_O_OneOfCase_OneofFixed64:
+        msg.oneofFixed64 = 107;
+        break;
+      case Message3_O_OneOfCase_OneofSfixed32:
+        msg.oneofSfixed32 = 108;
+        break;
+      case Message3_O_OneOfCase_OneofSfixed64:
+        msg.oneofSfixed64 = 109;
+        break;
+      case Message3_O_OneOfCase_OneofFloat:
+        msg.oneofFloat = 110.0f;
+        break;
+      case Message3_O_OneOfCase_OneofDouble:
+        msg.oneofDouble = 111.0;
+        break;
+      case Message3_O_OneOfCase_OneofBool:
+        msg.oneofBool = YES;
+        break;
+      case Message3_O_OneOfCase_OneofString:
+        msg.oneofString = oneofStringDefault;
+        break;
+      case Message3_O_OneOfCase_OneofBytes:
+        msg.oneofBytes = oneofBytesDefault;
+        break;
+      case Message3_O_OneOfCase_OneofEnum:
+        msg.oneofEnum = Message3_Enum_Baz;
+        break;
+      default:
+        XCTFail(@"shouldn't happen, loop: %zd, value: %d", i, values[i]);
+        break;
+    }
+
+    // Should be set to the correct case.
+    XCTAssertEqual(msg.oOneOfCase, values[i], "Loop: %zd", i);
+
+    // Confirm everything is back as the defaults.
+    XCTAssertEqual(msg.oneofInt32, 100, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofInt64, 101, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofUint32, 102U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofUint64, 103U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSint32, 104, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSint64, 105, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofFixed32, 106U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofFixed64, 107U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSfixed32, 108, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSfixed64, 109, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofFloat, 110.0f, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofDouble, 111.0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofBool, YES, "Loop: %zd", i);
+    XCTAssertEqualObjects(msg.oneofString, oneofStringDefault, "Loop: %zd", i);
+    XCTAssertEqualObjects(msg.oneofBytes, oneofBytesDefault, "Loop: %zd", i);
+    XCTAssertNotNil(msg.oneofGroup, "Loop: %zd", i);
+    // Skip group
+    // Skip message
+    XCTAssertEqual(msg.oneofEnum, Message2_Enum_Baz, "Loop: %zd", i);
+  }
+
+  // We special case nil on string, data, message, ensure they work as expected.
+  // i.e. - it clears the case.
+  msg.oneofString = nil;
+  XCTAssertEqual(msg.oOneOfCase, Message3_O_OneOfCase_GPBUnsetOneOfCase);
+  msg.oneofBytes = nil;
+  XCTAssertEqual(msg.oOneOfCase, Message3_O_OneOfCase_GPBUnsetOneOfCase);
+  msg.oneofMessage = nil;
+  XCTAssertEqual(msg.oOneOfCase, Message3_O_OneOfCase_GPBUnsetOneOfCase);
+
+  [msg release];
+}
+
+- (void)testProto3OneofSetToZero {
+
+  // Normally setting a proto3 field to the zero value should result in it being
+  // reset/cleared.  But in a oneof, it still gets recored so it can go out
+  // over the wire and the other side can see what was set in the oneof.
+
+  NSString *oneofStringDefault = @"";
+  NSData *oneofBytesDefault = [NSData data];
+
+  Message3 *msg = [[Message3 alloc] init];
+
+  uint32_t values[] = {
+    Message3_O_OneOfCase_OneofInt32,
+    Message3_O_OneOfCase_OneofInt64,
+    Message3_O_OneOfCase_OneofUint32,
+    Message3_O_OneOfCase_OneofUint64,
+    Message3_O_OneOfCase_OneofSint32,
+    Message3_O_OneOfCase_OneofSint64,
+    Message3_O_OneOfCase_OneofFixed32,
+    Message3_O_OneOfCase_OneofFixed64,
+    Message3_O_OneOfCase_OneofSfixed32,
+    Message3_O_OneOfCase_OneofSfixed64,
+    Message3_O_OneOfCase_OneofFloat,
+    Message3_O_OneOfCase_OneofDouble,
+    Message3_O_OneOfCase_OneofBool,
+    Message3_O_OneOfCase_OneofString,
+    Message3_O_OneOfCase_OneofBytes,
+    Message3_O_OneOfCase_OneofMessage,
+    Message3_O_OneOfCase_OneofEnum,
+  };
+
+  for (size_t i = 0; i < GPBARRAYSIZE(values); ++i) {
+    switch (values[i]) {
+      case Message3_O_OneOfCase_OneofInt32:
+        msg.oneofInt32 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofInt64:
+        msg.oneofInt64 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofUint32:
+        msg.oneofUint32 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofUint64:
+        msg.oneofUint64 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofSint32:
+        msg.oneofSint32 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofSint64:
+        msg.oneofSint64 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofFixed32:
+        msg.oneofFixed32 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofFixed64:
+        msg.oneofFixed64 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofSfixed32:
+        msg.oneofSfixed32 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofSfixed64:
+        msg.oneofSfixed64 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofFloat:
+        msg.oneofFloat = 0.0f;
+        break;
+      case Message3_O_OneOfCase_OneofDouble:
+        msg.oneofDouble = 0.0;
+        break;
+      case Message3_O_OneOfCase_OneofBool:
+        msg.oneofBool = NO;
+        break;
+      case Message3_O_OneOfCase_OneofString:
+        msg.oneofString = oneofStringDefault;
+        break;
+      case Message3_O_OneOfCase_OneofBytes:
+        msg.oneofBytes = oneofBytesDefault;
+        break;
+      case Message3_O_OneOfCase_OneofMessage:
+        msg.oneofMessage.optionalInt32 = 0;
+        break;
+      case Message3_O_OneOfCase_OneofEnum:
+        msg.oneofEnum = Message3_Enum_Foo;
+        break;
+      default:
+        XCTFail(@"shouldn't happen, loop: %zd, value: %d", i, values[i]);
+        break;
+    }
+
+    // Should be set to the correct case.
+    XCTAssertEqual(msg.oOneOfCase, values[i], "Loop: %zd", i);
+
+    // Confirm everything is still zeros.
+    XCTAssertEqual(msg.oneofInt32, 0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofInt64, 0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofUint32, 0U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofUint64, 0U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSint32, 0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSint64, 0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofFixed32, 0U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofFixed64, 0U, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSfixed32, 0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofSfixed64, 0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofFloat, 0.0f, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofDouble, 0.0, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofBool, NO, "Loop: %zd", i);
+    XCTAssertEqualObjects(msg.oneofString, oneofStringDefault, "Loop: %zd", i);
+    XCTAssertEqualObjects(msg.oneofBytes, oneofBytesDefault, "Loop: %zd", i);
+    XCTAssertNotNil(msg.oneofMessage, "Loop: %zd", i);
+    XCTAssertEqual(msg.oneofEnum, Message3_Enum_Foo, "Loop: %zd", i);
+  }
+
+  // We special case nil on string, data, message, ensure they work as expected.
+  msg.oneofString = nil;
+  XCTAssertEqual(msg.oOneOfCase, Message3_O_OneOfCase_GPBUnsetOneOfCase);
+  msg.oneofBytes = nil;
+  XCTAssertEqual(msg.oOneOfCase, Message3_O_OneOfCase_GPBUnsetOneOfCase);
+  msg.oneofMessage = nil;
+  XCTAssertEqual(msg.oOneOfCase, Message3_O_OneOfCase_GPBUnsetOneOfCase);
+
+  [msg release];
+}
+
 - (void)testCopyingMakesUniqueObjects {
  const int repeatCount = 5;
  TestAllTypes *msg1 = [TestAllTypes message];