Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions internal/gengapic/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ var enableWrapperTypesForPageSize = map[string]bool{
"google.cloud.bigquery.v2": true,
}

var enableOrderedRoutingHeaders = map[string]bool{
"google.firestore.v1": true,
"google.firestore.admin.v1": true,
}

var enableMtlsHardBoundTokens = map[string]bool{
"bigquery.googleapis.com": true,
"cloudasset.googleapis.com": true,
Expand Down
23 changes: 20 additions & 3 deletions internal/gengapic/gengapic.go
Original file line number Diff line number Diff line change
Expand Up @@ -457,9 +457,26 @@ func (g *generator) insertDynamicRequestHeaders(m *descriptorpb.MethodDescriptor
g.printf(" routingHeadersMap[%q] = %s", headerName, regexHelper)
g.printf("}")
}
g.printf("for headerName, headerValue := range routingHeadersMap {")
g.printf(` routingHeaders = fmt.Sprintf("%%s%%s=%%s&", routingHeaders, headerName, headerValue)`)
g.printf("}")
var orderedHeaders bool
for p, ok := range enableOrderedRoutingHeaders {
if g.descInfo.ParentFile[m].GetPackage() == p && ok {
orderedHeaders = true
break
}
}
if orderedHeaders {
for i := range headers {
headerName := headers[i][2]
g.printf("if headerValue, ok := routingHeadersMap[%q]; ok {", headerName)
g.printf(` routingHeaders = fmt.Sprintf("%%s%%s=%%s&", routingHeaders, %q, headerValue)`, headerName)
g.printf(" delete(routingHeadersMap, %q)", headerName)
g.printf("}")
}
} else {
g.printf("for headerName, headerValue := range routingHeadersMap {")
g.printf(` routingHeaders = fmt.Sprintf("%%s%%s=%%s&", routingHeaders, headerName, headerValue)`)
g.printf("}")
}
g.printf(`routingHeaders = strings.TrimSuffix(routingHeaders, "&")`)
g.imports[pbinfo.ImportSpec{Path: "strings"}] = true
g.printf(`hds := []string{"x-goog-request-params", routingHeaders}`)
Expand Down
108 changes: 108 additions & 0 deletions internal/gengapic/gengapic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1414,6 +1414,114 @@ func TestGenAndCommentHelpers(t *testing.T) {
}
}

func TestInsertDynamicRequestHeaders_Ordering(t *testing.T) {
g := &generator{
imports: make(map[pbinfo.ImportSpec]bool),
descInfo: pbinfo.Info{
Type: make(map[string]pbinfo.ProtoType),
ParentFile: make(map[protoreflect.ProtoMessage]*descriptorpb.FileDescriptorProto),
},
}

m := &descriptorpb.MethodDescriptorProto{
Name: proto.String("TestMethod"),
InputType: proto.String(".InputType"),
Options: &descriptorpb.MethodOptions{},
}

// Setup InputType with fields used in headers
inputType := &descriptorpb.DescriptorProto{
Name: proto.String("InputType"),
Field: []*descriptorpb.FieldDescriptorProto{
{Name: proto.String("field1"), Type: typePtr(descriptorpb.FieldDescriptorProto_TYPE_STRING)},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure of this typePtr usage. Can't you use https://pkg.go.dev/google.golang.org/protobuf/types/descriptorpb#FieldDescriptorProto_Type.Enum e.g. descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing in #1686

{Name: proto.String("field2"), Type: typePtr(descriptorpb.FieldDescriptorProto_TYPE_STRING)},
{Name: proto.String("field3"), Type: typePtr(descriptorpb.FieldDescriptorProto_TYPE_STRING)},
},
}
g.descInfo.Type[".InputType"] = inputType

// The input headers list defines the order:
// 1. header_a (from field1)
// 2. header_b (from field2)
// 3. header_a (from field3) -- Duplicate key
headers := [][]string{
{"regex1", "field1", "header_a"},
{"regex2", "field2", "header_b"},
{"regex3", "field3", "header_a"},
}

tests := []struct {
pkgName string
wantOrdered bool
}{
{"google.firestore.v1", true},
{"google.firestore.admin.v1", true},
{"google.cloud.storage.v1", false},
{"other.package", false},
}

for _, tc := range tests {
t.Run(tc.pkgName, func(t *testing.T) {
g.reset()

file := &descriptorpb.FileDescriptorProto{
Package: proto.String(tc.pkgName),
}
g.descInfo.ParentFile[m] = file
g.descInfo.ParentFile[inputType] = file

err := g.insertDynamicRequestHeaders(m, headers)
if err != nil {
t.Fatalf("insertDynamicRequestHeaders failed: %v", err)
}

got := g.pt.String()

if tc.wantOrdered {
// Verify strict order of checks in the generated code.
// We expect code blocks for header_a, then header_b, then header_a again.

idxA1 := strings.Index(got, `if headerValue, ok := routingHeadersMap["header_a"]; ok {`)
if idxA1 == -1 {
t.Errorf("expected first check for header_a")
}

// Look for header_b check AFTER header_a check
idxB := strings.Index(got[idxA1+1:], `if headerValue, ok := routingHeadersMap["header_b"]; ok {`)
if idxB == -1 {
t.Errorf("expected check for header_b after first header_a")
}
idxB += idxA1 + 1 // Adjust index to be absolute

// Look for second header_a check AFTER header_b check
idxA2 := strings.Index(got[idxB+1:], `if headerValue, ok := routingHeadersMap["header_a"]; ok {`)
if idxA2 == -1 {
t.Errorf("expected second check for header_a after header_b")
}

// Verify delete is called
if !strings.Contains(got, `delete(routingHeadersMap, "header_a")`) {
t.Errorf("expected delete map key for %s", tc.pkgName)
}

// Verify absence of range loop
if strings.Contains(got, `range routingHeadersMap`) {
t.Errorf("expected NO range map iteration for %s", tc.pkgName)
}
} else {
// Check for unordered iteration pattern: range loop
if !strings.Contains(got, `for headerName, headerValue := range routingHeadersMap {`) {
t.Errorf("expected range map iteration for %s, got:\n%s", tc.pkgName, got)
}
}
})
}
}

func typePtr(t descriptorpb.FieldDescriptorProto_Type) *descriptorpb.FieldDescriptorProto_Type {
return &t
}

func setHTTPOption(o *descriptorpb.MethodOptions, pattern string) {
proto.SetExtension(o, annotations.E_Http, &annotations.HttpRule{
Pattern: &annotations.HttpRule_Get{
Expand Down
Loading