fix(openapiv2): prevent nested required fields hoisting to parent schema

When generating OpenAPI schemas for request bodies with path parameters
extracted from nested messages (e.g., {thing.name}), nested required
field names were incorrectly added to the parent body schemas required
array instead of remaining in the nested objects required array.

For example, with UpdateFooRequest containing a required Foo field, and
Foo having required name and value fields, when using path parameter
{thing.name}, the generated body schema incorrectly had:
  required: ["value", "thing"]
Instead of:
  required: ["thing"] with thing.required: ["value"]

This fix adds logic to distinguish between field-level and nested-level
required markers when path parameters are present. It ensures only the
field name itself appears in the parents required array if the field
is marked REQUIRED, while nested field names remain in the nested
schemas required array.

Author: rdark

protoc-gen-openapiv2/internal/genopenapi/template.go

@@ -613,11 +613,38 @@ func renderMessageAsDefinition(msg *descriptor.Message, reg *descriptor.Registry
 		}
 
 		if fieldSchema.Required != nil {
-			schema.Required = getUniqueFields(schema.Required, fieldSchema.Required)
-			schema.Required = append(schema.Required, fieldSchema.Required...)
-			// To avoid populating both the field schema require and message schema require, unset the field schema require.
-			// See issue #2635.
-			fieldSchema.Required = nil
+			// Only hoist required fields to parent if there are no path params inside this field.
+			if len(subPathParams) == 0 {
+				schema.Required = getUniqueFields(schema.Required, fieldSchema.Required)
+				schema.Required = append(schema.Required, fieldSchema.Required...)
+				// To avoid populating both the field schema require and message schema require, unset the field schema require.
+				// See issue #2635.
+				fieldSchema.Required = nil
+			} else {
+				// When there are path params, we need to separate field-level required from nested required.
+				// The field name itself (if required) should be in parent's required, but nested field names
+				// should stay in the nested schema's required.
+				fieldName := f.GetName()
+				if reg.GetUseJSONNamesForFields() {
+					fieldName = f.GetJsonName()
+				}
+				// Check if the field name is in the fieldSchema.Required (it would be if the field is marked REQUIRED)
+				var nestedRequired []string
+				fieldIsRequired := false
+				for _, req := range fieldSchema.Required {
+					if req == fieldName {
+						fieldIsRequired = true
+					} else {
+						nestedRequired = append(nestedRequired, req)
+					}
+				}
+				// Add the field name to parent's required if the field itself is required
+				if fieldIsRequired && find(schema.Required, fieldName) == -1 {
+					schema.Required = append(schema.Required, fieldName)
+				}
+				// Keep only the nested required fields in the field schema
+				fieldSchema.Required = nestedRequired
+			}
 		}
 
 		if reg.GetUseAllOfForRefs() {

protoc-gen-openapiv2/internal/genopenapi/template_test.go

@@ -11784,3 +11784,226 @@ func Test_updateSwaggerObjectFromFieldBehavior(t *testing.T) {
 		})
 	}
 }
+
+// TestNestedRequiredFieldsNotHoisted tests the bug where nested required fields
+// are incorrectly hoisted to the parent body schema's required array when path
+// parameters reference nested fields
+func TestNestedRequiredFieldsNotHoisted(t *testing.T) {
+	fieldBehaviorRequired := []annotations.FieldBehavior{annotations.FieldBehavior_REQUIRED}
+	requiredFieldOptions := new(descriptorpb.FieldOptions)
+	proto.SetExtension(requiredFieldOptions, annotations.E_FieldBehavior, fieldBehaviorRequired)
+
+	// Define the nested message (Foo) with REQUIRED fields
+	fooDesc := &descriptorpb.DescriptorProto{
+		Name: proto.String("Foo"),
+		Field: []*descriptorpb.FieldDescriptorProto{
+			{
+				Name:    proto.String("name"),
+				Type:    descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(),
+				Number:  proto.Int32(1),
+				Options: requiredFieldOptions, // name is REQUIRED
+			},
+			{
+				Name:    proto.String("value"),
+				Type:    descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(),
+				Number:  proto.Int32(2),
+				Options: requiredFieldOptions, // value is REQUIRED
+			},
+		},
+	}
+
+	// Define the request message (UpdateFooRequest)
+	updateFooReqDesc := &descriptorpb.DescriptorProto{
+		Name: proto.String("UpdateFooRequest"),
+		Field: []*descriptorpb.FieldDescriptorProto{
+			{
+				Name:     proto.String("thing"),
+				Type:     descriptorpb.FieldDescriptorProto_TYPE_MESSAGE.Enum(),
+				TypeName: proto.String(".test.Foo"),
+				Number:   proto.Int32(1),
+				Options:  requiredFieldOptions, // thing is REQUIRED
+			},
+			{
+				Name:   proto.String("update_mask"),
+				Type:   descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(), // Simplified - normally FieldMask
+				Number: proto.Int32(2),
+			},
+		},
+	}
+
+	fooMsg := &descriptor.Message{
+		DescriptorProto: fooDesc,
+	}
+
+	updateFooReqMsg := &descriptor.Message{
+		DescriptorProto: updateFooReqDesc,
+	}
+
+	nameField := &descriptor.Field{
+		Message:              fooMsg,
+		FieldDescriptorProto: fooMsg.GetField()[0],
+	}
+	valueField := &descriptor.Field{
+		Message:              fooMsg,
+		FieldDescriptorProto: fooMsg.GetField()[1],
+	}
+	fooMsg.Fields = []*descriptor.Field{nameField, valueField}
+
+	thingField := &descriptor.Field{
+		Message:              updateFooReqMsg,
+		FieldMessage:         fooMsg,
+		FieldDescriptorProto: updateFooReqMsg.GetField()[0],
+	}
+	updateMaskField := &descriptor.Field{
+		Message:              updateFooReqMsg,
+		FieldDescriptorProto: updateFooReqMsg.GetField()[1],
+	}
+	updateFooReqMsg.Fields = []*descriptor.Field{thingField, updateMaskField}
+
+	meth := &descriptorpb.MethodDescriptorProto{
+		Name:       proto.String("UpdateFoo"),
+		InputType:  proto.String("UpdateFooRequest"),
+		OutputType: proto.String("Foo"),
+	}
+
+	svc := &descriptorpb.ServiceDescriptorProto{
+		Name:   proto.String("FooService"),
+		Method: []*descriptorpb.MethodDescriptorProto{meth},
+	}
+
+	file := descriptor.File{
+		FileDescriptorProto: &descriptorpb.FileDescriptorProto{
+			SourceCodeInfo: &descriptorpb.SourceCodeInfo{},
+			Name:           proto.String("foo.proto"),
+			Package:        proto.String("test"),
+			MessageType:    []*descriptorpb.DescriptorProto{fooDesc, updateFooReqDesc},
+			Service:        []*descriptorpb.ServiceDescriptorProto{svc},
+			Options: &descriptorpb.FileOptions{
+				GoPackage: proto.String("github.com/example/test;test"),
+			},
+		},
+		GoPkg: descriptor.GoPackage{
+			Path: "example.com/path/to/test/test.pb",
+			Name: "test_pb",
+		},
+		Messages: []*descriptor.Message{fooMsg, updateFooReqMsg},
+		Services: []*descriptor.Service{
+			{
+				ServiceDescriptorProto: svc,
+				Methods: []*descriptor.Method{
+					{
+						MethodDescriptorProto: meth,
+						RequestType:           updateFooReqMsg,
+						ResponseType:          fooMsg,
+						Bindings: []*descriptor.Binding{
+							{
+								HTTPMethod: "PATCH",
+								PathTmpl: httprule.Template{
+									Version:  1,
+									OpCodes:  []int{0, 0},
+									Template: "/api/v1/{thing.name}",
+								},
+								PathParams: []descriptor.Parameter{
+									{
+										FieldPath: descriptor.FieldPath([]descriptor.FieldPathComponent{
+											{
+												Name: "thing",
+											},
+											{
+												Name: "name",
+											},
+										}),
+										Target: nameField,
+									},
+								},
+								Body: &descriptor.Body{
+									FieldPath: []descriptor.FieldPathComponent{}, // body: "*"
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	reg := descriptor.NewRegistry()
+	fileCL := crossLinkFixture(&file)
+	err := reg.Load(reqFromFile(fileCL))
+	if err != nil {
+		t.Errorf("reg.Load(%#v) failed with %v; want success", file, err)
+		return
+	}
+
+	result, err := applyTemplate(param{File: fileCL, reg: reg})
+	if err != nil {
+		t.Fatalf("applyTemplate(%#v) failed with %v; want success", file, err)
+	}
+
+	paths := GetPaths(result)
+	if got, want := len(paths), 1; got != want {
+		t.Fatalf("Results path length differed, got %d want %d", got, want)
+	}
+	if got, want := paths[0], "/api/v1/{thing.name}"; got != want {
+		t.Fatalf("Wrong results path, got %s want %s", got, want)
+	}
+
+	operation := *result.getPathItemObject("/api/v1/{thing.name}").Patch
+	if len(operation.Parameters) < 2 {
+		t.Fatalf("Expected at least 2 parameters, got %d", len(operation.Parameters))
+	}
+
+	if got, want := operation.Parameters[0].Name, "thing.name"; got != want {
+		t.Fatalf("Wrong parameter name 0, got %s want %s", got, want)
+	}
+	if got, want := operation.Parameters[0].In, "path"; got != want {
+		t.Fatalf("Wrong parameter location 0, got %s want %s", got, want)
+	}
+
+	if got, want := operation.Parameters[1].Name, "body"; got != want {
+		t.Fatalf("Wrong parameter name 1, got %s want %s", got, want)
+	}
+	if got, want := operation.Parameters[1].In, "body"; got != want {
+		t.Fatalf("Wrong parameter location 1, got %s want %s", got, want)
+	}
+
+	bodySchemaRef := operation.Parameters[1].Schema.schemaCore.Ref
+	if bodySchemaRef == "" {
+		t.Fatal("Body schema reference is empty")
+	}
+
+	defName := strings.TrimPrefix(bodySchemaRef, "#/definitions/")
+	definition, found := result.Definitions[defName]
+	if !found {
+		t.Fatalf("expecting definition to contain %s", defName)
+	}
+
+	// Verify that nested required fields are NOT hoisted to parent level
+	correctRequiredFields := []string{"thing"}
+	if got, want := definition.Required, correctRequiredFields; !reflect.DeepEqual(got, want) {
+		t.Errorf("Nested required fields were incorrectly hoisted to parent level.\n"+
+			"Body definition required fields:\n"+
+			"  got  = %v\n"+
+			"  want = %v (only top-level field names)\n"+
+			"Nested field 'value' should be in 'thing' property's required array, not parent's.",
+			got, want)
+	}
+
+	var thingKV *keyVal
+	if definition.Properties != nil {
+		for i := range *definition.Properties {
+			if (*definition.Properties)[i].Key == "thing" {
+				thingKV = &(*definition.Properties)[i]
+				break
+			}
+		}
+	}
+
+	if thingKV == nil {
+		t.Fatal("'thing' property not found in body definition")
+	}
+
+	if _, ok := thingKV.Value.(openapiSchemaObject); !ok {
+		t.Fatal("'thing' property value is not an openapiSchemaObject")
+	}
+}