Expecting a oneof validation error but only getting it intermittently

[jamesdavidson]

🐛 Bug Report

When receiving values from query string params, fields that belong to a oneof are not validated consistently.

To Reproduce

Let’s say you have an Outer message with fields foo and bar which form a oneof and foo is of type int32 and bar is a nested message type called Inner.

message Outer {
  oneof value {
    int32 foo = 1;
    Inner bar = 2;
  }
  message Inner {
    int32 width = 1;
    int32 height = 2;
  }
}

If QueryParameterParser tries to build a message from a GET request by parsing query parameters in the URL then sometimes it fails to assert the oneof condition: either foo can have a value, or bar can have a value, but not both.

Example requests:

http://example.com/endpoint?foo=123  // okay
http://example.com/endpoint?bar.width=100&bar.height=100  // okay
http://example.com/endpoint?bar.width=100&bar.height=100&foo=123   // not okay. should always get the validation error "field already set for oneof"

For the last example, sometimes you get a validation error, sometimes you don’t.

Expected behavior

Should always get a "field already set for oneof" validation error when foo and bar are both present in the URL query string.

Actual Behavior

Sometimes you get a "field already set for oneof" validation error, sometimes you don’t.

Your Environment

Version v2.20.0 on Ubuntu 22.04

[jamesdavidson]

The conditions under which we don’t get a validation error seem to be that if foo is parsed before bar then the value received from foo is cleared as a side-effect of parsing bar.width and bar.height (so the validation is unaware of foo's prior value).

The culprit appears to be msgValue = msgValue.Mutable(fieldDescriptor).Message() . Why would getting a reference to a field cause adjacent values to be cleared? Well the docstring for protoreflect#Message.Mutable includes this ominous warning:

For a field belonging to a oneof, it implicitly clears any other field that may be currently set within the same oneof.

We have a workaround (using runtime.SetQueryParameterParser) but hopefully this can be fixed. One approach would be to move the "Check if oneof already set" bit of code into the for loop above the update to msgValue.