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

feat: add expand_slashed_path_patterns flag #4813

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
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
20 changes: 20 additions & 0 deletions internal/descriptor/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,18 @@ type Registry struct {

// enableRpcDeprecation whether to process grpc method's deprecated option
enableRpcDeprecation bool

// expandSlashedPathPatterns, if true, for a path parameter carying a sub-path, described via parameter pattern (i.e.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// expandSlashedPathPatterns, if true, for a path parameter carying a sub-path, described via parameter pattern (i.e.
// expandSlashedPathPatterns, if true, for a path parameter carrying a sub-path, described via parameter pattern (i.e.

// the pattern contains forward slashes), this will expand the _pattern_ into the URI and will _replace_ the parameter
// with new path parameters inferred from patterns wildcards.
//
// Example: a Google AIP style path "/v1/{name=projects/*/locations/*}/datasets/{dataset}" with a "name" parameter
// containing sub-path will generate "/v1/projects/{project}/locations/{location}/datasets/{dataset}" path in OpenAPI.
// Note that the original "name" parameter is replaced with "project" and "location" parameters.
//
// This leads to more compliant and readable OpenAPI suitable for documentation, but may complicate client
// implementation if you want to pass the original "name" parameter.
expandSlashedPathPatterns bool
}

type repeatedFieldSeparator struct {
Expand Down Expand Up @@ -888,3 +900,11 @@ func (r *Registry) SetEnableRpcDeprecation(enable bool) {
func (r *Registry) GetEnableRpcDeprecation() bool {
return r.enableRpcDeprecation
}

func (r *Registry) SetExpandSlashedPathPatterns(expandSlashedPathPatterns bool) {
r.expandSlashedPathPatterns = expandSlashedPathPatterns
}

func (r *Registry) GetExpandSlashedPathPatterns() bool {
return r.expandSlashedPathPatterns
}
14 changes: 13 additions & 1 deletion protoc-gen-openapiv2/defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ def _run_proto_gen_openapi(
visibility_restriction_selectors,
use_allof_for_refs,
disable_default_responses,
enable_rpc_deprecation):
enable_rpc_deprecation,
expand_slashed_path_patterns):
args = actions.args()

args.add("--plugin", "protoc-gen-openapiv2=%s" % protoc_gen_openapiv2.path)
Expand Down Expand Up @@ -152,6 +153,9 @@ def _run_proto_gen_openapi(
if enable_rpc_deprecation:
args.add("--openapiv2_opt", "enable_rpc_deprecation=true")

if expand_slashed_path_patterns:
args.add("--openapiv2_opt", "expand_slashed_path_patterns=true")

args.add("--openapiv2_opt", "repeated_path_param_separator=%s" % repeated_path_param_separator)

proto_file_infos = _direct_source_infos(proto_info)
Expand Down Expand Up @@ -260,6 +264,7 @@ def _proto_gen_openapi_impl(ctx):
use_allof_for_refs = ctx.attr.use_allof_for_refs,
disable_default_responses = ctx.attr.disable_default_responses,
enable_rpc_deprecation = ctx.attr.enable_rpc_deprecation,
expand_slashed_path_patterns = ctx.attr.expand_slashed_path_patterns,
),
),
),
Expand Down Expand Up @@ -420,6 +425,13 @@ protoc_gen_openapiv2 = rule(
mandatory = False,
doc = "whether to process grpc method's deprecated option.",
),
"expand_slashed_path_patterns": attr.bool(
default = False,
mandatory = False,
doc = "if set, expands path patterns containing slashes into URI." +
" It also creates a new path parameter for each wildcard in " +
" the path pattern.",
),
"_protoc": attr.label(
default = "@com_google_protobuf//:protoc",
executable = True,
Expand Down
93 changes: 90 additions & 3 deletions protoc-gen-openapiv2/internal/genopenapi/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"os"
"reflect"
"regexp"
"slices"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -977,8 +978,17 @@ func resolveFullyQualifiedNameToOpenAPINames(messages []string, namingStrategy s

var canRegexp = regexp.MustCompile("{([a-zA-Z][a-zA-Z0-9_.]*)([^}]*)}")

// templateToParts will split a URL template as defined by https://github.com/googleapis/googleapis/blob/master/google/api/http.proto
// into a string slice with each part as an element of the slice for use by `partsToOpenAPIPath` and `partsToRegexpMap`.
// templateToParts splits a URL template into path segments for use by `partsToOpenAPIPath` and `partsToRegexpMap`.
//
// Parameters:
// - path: The URL template as defined by https://github.com/googleapis/googleapis/blob/master/google/api/http.proto
// - reg: The descriptor registry used to read compiler flags
// - fields: The fields of the request message, only used when `useJSONNamesForFields` is true
// - msgs: The Messages of the service binding, only used when `useJSONNamesForFields` is true
//
// Returns:
//
// The path segments of the URL template.
func templateToParts(path string, reg *descriptor.Registry, fields []*descriptor.Field, msgs []*descriptor.Message) []string {
// It seems like the right thing to do here is to just use
// strings.Split(path, "/") but that breaks badly when you hit a url like
Expand Down Expand Up @@ -1053,6 +1063,7 @@ pathLoop:
func partsToOpenAPIPath(parts []string, overrides map[string]string) string {
for index, part := range parts {
part = canRegexp.ReplaceAllString(part, "{$1}")

if override, ok := overrides[part]; ok {
part = override
}
Expand Down Expand Up @@ -1145,6 +1156,78 @@ func renderServiceTags(services []*descriptor.Service, reg *descriptor.Registry)
return tags
}

// expandPathPatterns search the URI parts for path parameters with pattern and when the pattern contains a sub-path,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// expandPathPatterns search the URI parts for path parameters with pattern and when the pattern contains a sub-path,
// expandPathPatterns searches the URI parts for path parameters with pattern and when the pattern contains a sub-path,

// it expands the pattern into the URI parts and adds the new path parameters to the pathParams slice.
//
// Parameters:
// - pathParts: the URI parts parsed from the path template with `templateToParts` function
// - pathParams: the path parameters of the service binding, this slice will be mutated when the path pattern contains
// a sub-path with wildcard.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// a sub-path with wildcard.
// a sub-path with a wildcard.

//
// Returns:
//
// The modified pathParts. Also mutates the pathParams slice.
func expandPathPatterns(pathParts []string, pathParams *[]descriptor.Parameter) []string {
Comment on lines +1169 to +1170
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this mutates an input, what do you think about returning a parameter of the same type instead? It's a bit gross IMO to mutate the input slice and returning it will make it more explicit.

Suggested change
// The modified pathParts. Also mutates the pathParams slice.
func expandPathPatterns(pathParts []string, pathParams *[]descriptor.Parameter) []string {
// The modified pathParts and pathParams slice.
func expandPathPatterns(pathParts []string, pathParams []descriptor.Parameter) ([]string, []descriptor.Parameter) {

Comment on lines +1169 to +1170
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realized that we have a package for parsing these paths already: https://github.com/grpc-ecosystem/grpc-gateway/blob/main/internal/httprule. What do you think about reusing this?

expandedPathParts := []string{}
for _, pathPart := range pathParts {
if !strings.HasPrefix(pathPart, "{") || !strings.HasSuffix(pathPart, "}") {
expandedPathParts = append(expandedPathParts, pathPart)
continue
}
woBraces := pathPart[1 : len(pathPart)-1]
paramPattern := strings.SplitN(woBraces, "=", 2)
if len(paramPattern) != 2 {
expandedPathParts = append(expandedPathParts, pathPart)
continue
}
paramName := paramPattern[0]
pattern := paramPattern[1]
if pattern == "*" {
expandedPathParts = append(expandedPathParts, pathPart)
continue
}
pathParamIndex := slices.IndexFunc(*pathParams, func(p descriptor.Parameter) bool {
return p.FieldPath.String() == paramName
})
if pathParamIndex == -1 {
panic(fmt.Sprintf("Path parameter %q not found in path parameters", paramName))
}
pathParam := (*pathParams)[pathParamIndex]
patternParts := strings.Split(pattern, "/")
for _, patternPart := range patternParts {
if patternPart != "*" {
expandedPathParts = append(expandedPathParts, patternPart)
continue
}
lastPart := expandedPathParts[len(expandedPathParts)-1]
paramName := strings.TrimSuffix(lastPart, "s")
expandedPathParts = append(expandedPathParts, "{"+paramName+"}")
newParam := descriptor.Parameter{
Target: &descriptor.Field{
FieldDescriptorProto: &descriptorpb.FieldDescriptorProto{
Name: proto.String(paramName),
},
Message: pathParam.Target.Message,
FieldMessage: pathParam.Target.FieldMessage,
ForcePrefixedName: pathParam.Target.ForcePrefixedName,
},
FieldPath: []descriptor.FieldPathComponent{{
Name: paramName,
Target: nil,
}},
Method: nil,
}
*pathParams = append((*pathParams), newParam)
if pathParamIndex != -1 {
// the new parameter from the pattern replaces the old path parameter
*pathParams = append((*pathParams)[:pathParamIndex], (*pathParams)[pathParamIndex+1:]...)
pathParamIndex = -1
}
}
}
return expandedPathParts
}

func renderServices(services []*descriptor.Service, paths *openapiPathsObject, reg *descriptor.Registry, requestResponseRefs, customRefs refMap, msgs []*descriptor.Message, defs openapiDefinitionsObject) error {
// Correctness of svcIdx and methIdx depends on 'services' containing the services in the same order as the 'file.Service' array.
svcBaseIdx := 0
Expand All @@ -1170,13 +1253,17 @@ func renderServices(services []*descriptor.Service, paths *openapiPathsObject, r
operationFunc := operationForMethod(b.HTTPMethod)
// Iterate over all the OpenAPI parameters
parameters := openapiParametersObject{}
pathParams := b.PathParams
// split the path template into its parts
parts := templateToParts(b.PathTmpl.Template, reg, meth.RequestType.Fields, msgs)
if reg.GetExpandSlashedPathPatterns() {
parts = expandPathPatterns(parts, &pathParams)
}
// extract any constraints specified in the path placeholders into ECMA regular expressions
pathParamRegexpMap := partsToRegexpMap(parts)
// Keep track of path parameter overrides
var pathParamNames = make(map[string]string)
for _, parameter := range b.PathParams {
for _, parameter := range pathParams {

var paramType, paramFormat, desc, collectionFormat string
var defaultValue interface{}
Expand Down
59 changes: 59 additions & 0 deletions protoc-gen-openapiv2/internal/genopenapi/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4119,6 +4119,61 @@ func TestTemplateToOpenAPIPath(t *testing.T) {
}
}

func getParameters(names []string) []descriptor.Parameter {
params := make([]descriptor.Parameter, 0)
for _, name := range names {
params = append(params, descriptor.Parameter{
Target: &descriptor.Field{
FieldDescriptorProto: &descriptorpb.FieldDescriptorProto{
Name: proto.String(name),
},
Message: &descriptor.Message{},
FieldMessage: nil,
ForcePrefixedName: false,
},
FieldPath: []descriptor.FieldPathComponent{{
Name: name,
Target: nil,
}},
Method: nil,
})
}
return params
}

func TestTemplateToOpenAPIPathExpandSlashed(t *testing.T) {
var tests = []struct {
input string
expected string
pathParams []descriptor.Parameter
expectedPathParams []string
useJSONNames bool
}{
{"/v1/{name=projects/*/documents/*}:exportResults", "/v1/projects/{project}/documents/{document}:exportResults", getParameters([]string{"name"}), []string{"project", "document"}, true},
{"/test/{name=*}", "/test/{name}", getParameters([]string{"name"}), []string{"name"}, true},
{"/test/{name=*}/", "/test/{name}/", getParameters([]string{"name"}), []string{"name"}, true},
{"/test/{name=test_cases/*}/", "/test/test_cases/{testCase}/", getParameters([]string{"name"}), []string{"testCase"}, true},
{"/test/{name=test_cases/*}/", "/test/test_cases/{test_case}/", getParameters([]string{"name"}), []string{"test_case"}, false},
}
reg := descriptor.NewRegistry()
reg.SetExpandSlashedPathPatterns(true)
for _, data := range tests {
reg.SetUseJSONNamesForFields(data.useJSONNames)
actual := templateToExpandedPath(data.input, reg, generateFieldsForJSONReservedName(), generateMsgsForJSONReservedName(), &data.pathParams)
if data.expected != actual {
t.Errorf("Expected templateToOpenAPIPath(%v) = %v, actual: %v", data.input, data.expected, actual)
}
pathParamsNames := make([]string, 0)
for _, param := range data.pathParams {
pathParamsNames = append(pathParamsNames, param.FieldPath[0].Name)
}
if !reflect.DeepEqual(data.expectedPathParams, pathParamsNames) {
t.Errorf("Expected mutated path params in templateToOpenAPIPath(%v) = %v, actual: %v", data.input, data.expectedPathParams, data.pathParams)
}

}
}

func BenchmarkTemplateToOpenAPIPath(b *testing.B) {
const input = "/{user.name=prefix1/*/prefix2/*}:customMethod"

Expand Down Expand Up @@ -4232,6 +4287,10 @@ func templateToRegexpMap(path string, reg *descriptor.Registry, fields []*descri
return partsToRegexpMap(templateToParts(path, reg, fields, msgs))
}

func templateToExpandedPath(path string, reg *descriptor.Registry, fields []*descriptor.Field, msgs []*descriptor.Message, pathParams *[]descriptor.Parameter) string {
return partsToOpenAPIPath(expandPathPatterns(templateToParts(path, reg, fields, msgs), pathParams), make(map[string]string))
}

func TestFQMNToRegexpMap(t *testing.T) {
var tests = []struct {
input string
Expand Down
2 changes: 2 additions & 0 deletions protoc-gen-openapiv2/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ var (
allowPatchFeature = flag.Bool("allow_patch_feature", true, "whether to hide update_mask fields in PATCH requests from the generated swagger file.")
preserveRPCOrder = flag.Bool("preserve_rpc_order", false, "if true, will ensure the order of paths emitted in openapi swagger files mirror the order of RPC methods found in proto files. If false, emitted paths will be ordered alphabetically.")
enableRpcDeprecation = flag.Bool("enable_rpc_deprecation", false, "whether to process grpc method's deprecated option.")
expandSlashedPathPatterns = flag.Bool("expand_slashed_path_patterns", false, "if set, expands path parameters with URI sub-paths into the URI. For example, \"/v1/{name=projects/*}/resource\" becomes \"/v1/projects/{project}/resource\".")

_ = flag.Bool("logtostderr", false, "Legacy glog compatibility. This flag is a no-op, you can safely remove it")
)
Expand Down Expand Up @@ -173,6 +174,7 @@ func main() {
reg.SetAllowPatchFeature(*allowPatchFeature)
reg.SetPreserveRPCOrder(*preserveRPCOrder)
reg.SetEnableRpcDeprecation(*enableRpcDeprecation)
reg.SetExpandSlashedPathPatterns(*expandSlashedPathPatterns)

if err := reg.SetRepeatedPathParamSeparator(*repeatedPathParamSeparator); err != nil {
emitError(err)
Expand Down
Loading