Skip to content

Commit

Permalink
feat(AIP-127): Validate HTTP pattern syntax (#1051)
Browse files Browse the repository at this point in the history
  • Loading branch information
acamadeo authored Dec 6, 2022
1 parent 2216d7d commit e55a765
Show file tree
Hide file tree
Showing 4 changed files with 258 additions and 0 deletions.
93 changes: 93 additions & 0 deletions docs/rules/0127/http-template-syntax.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
---
rule:
aip: 127
name: [core, '0127', http-template-syntax]
summary: |
HTTP patterns should follow the HTTP path template syntax.
permalink: /127/http-template-syntax
redirect_from:
- /127/http-template-syntax
---

# HTTP Pattern Variables

This rule enforces that HTTP annotation patterns follow the path template syntax
rules, as mandated in [AIP-127][].

## Details

This rule ensures that `google.api.http` patterns adhere to the following
[syntax rules]
(https://github.com/googleapis/googleapis/blob/83c3605afb5a39952bf0a0809875d41cf2a558ca/google/api/http.proto#L224).

## Examples

**Incorrect** code for this rule:

```proto
// Incorrect.
rpc GetBook(GetBookRequest) returns (Book) {
option (google.api.http) = {
// Should start with a leading slash.
get: "v1/{name=shelves/*}"
};
}
```

**Incorrect** code for this rule:

```proto
// Incorrect.
rpc AddAuthor(AddAuthorRequest) returns (AddAuthorResponse) {
option (google.api.http) = {
// Verb should be marked off with the ':' character.
post: "/v1/{book=publishers/*/books/*}-addAuthor"
body: "*"
};
}
```

**Incorrect** code for this rule:

```proto
// Incorrect.
rpc CreateBook(CreateBookRequest) returns (Book) {
option (google.api.http) = {
// The triple wildcard ('***') is not a part of the syntax.
post: "/v1/{parent=publishers/***}"
body: "book"
};
}
```

**Correct** code for this rule:

```proto
// Correct.
rpc GetBook(GetBookRequest) returns (Book) {
option (google.api.http) = {
get: "/v1/{name=shelves/*}"
};
}
```

## Disabling

If you need to violate this rule, use a leading comment above the field.
Remember to also include an [aip.dev/not-precedent][] comment explaining why.

```proto
// (-- api-linter: core::0127::http-template-syntax=disabled
// aip.dev/not-precedent: We need to do this because reasons. --)
rpc GetBook(GetBookRequest) returns (Book) {
option (google.api.http) = {
get: "v1/{name=shelves/*}"
};
}
```

If you need to violate this rule for an entire file, place the comment at the
top of the file.

[aip-127]: https://aip.dev/127
[aip.dev/not-precedent]: https://aip.dev/not-precedent
1 change: 1 addition & 0 deletions rules/aip0127/aip0127.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ func AddRules(r lint.RuleRegistry) error {
127,
hasAnnotation,
httpTemplatePattern,
httpTemplateSyntax,
leadingSlash,
resourceNameExtraction,
)
Expand Down
60 changes: 60 additions & 0 deletions rules/aip0127/http_template_syntax.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright 2022 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package aip0127

import (
"fmt"
"regexp"

"github.com/googleapis/api-linter/lint"
"github.com/googleapis/api-linter/locations"
"github.com/googleapis/api-linter/rules/internal/utils"
"github.com/jhump/protoreflect/desc"
)

var (
literal = `(\w+)`
identifier = `(\w+)`
verb = fmt.Sprintf("(:%s)", literal)
fieldPath = fmt.Sprintf(`(%s(\.%s)*)`, identifier, identifier)
variableSegment = fmt.Sprintf(`(\*\*|\*|%s)`, literal)
variableTemplate = fmt.Sprintf("(%s(/%s)*)", variableSegment, variableSegment)
variable = fmt.Sprintf(`(\{%s(=%s)?\})`, fieldPath, variableTemplate)
segment = fmt.Sprintf(`(\*\*|\*|%s|%s)`, literal, variable)
segments = fmt.Sprintf("(%s(/%s)*)", segment, segment)
template = fmt.Sprintf("^/%s(%s)?$", segments, verb)
templateRegex = regexp.MustCompile(template)
)

// HTTP URL pattern should follow the syntax rules described here:
// https://github.com/googleapis/googleapis/blob/16db2fb7fab4668bdfa09966513e03581d8f5e35/google/api/http.proto#L224.
var httpTemplateSyntax = &lint.MethodRule{
Name: lint.NewRuleName(127, "http-template-syntax"),
OnlyIf: utils.HasHTTPRules,
LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
problems := []lint.Problem{}
for _, httpRule := range utils.GetHTTPRules(m) {
if !templateRegex.MatchString(httpRule.URI) {
message := fmt.Sprintf("The HTTP pattern %q does not follow proper HTTP path template syntax", httpRule.URI)
problems = append(problems, lint.Problem{
Message: message,
Descriptor: m,
Location: locations.MethodHTTPRule(m),
})
}
}
return problems
},
}
104 changes: 104 additions & 0 deletions rules/aip0127/http_template_syntax_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Copyright 2022 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package aip0127

import (
"testing"

"github.com/googleapis/api-linter/rules/internal/testutils"
)

func TestHttpTemplateSyntax(t *testing.T) {
tests := []struct {
testName string
URI string
valid bool
}{
// NOTE: These examples are contrived to test the enforcment of the
// template syntax. Many of these examples either fail or do not make
// sense in the context of other AIP rules.

// Valid cases
{"SingleLiteral", "/v1", true},
{"TwoLiterals", "/v1/books", true},
{"ThreeLiterals", "/v1/books/shelves", true},
{"SingleLiteralWithVerb", "/v1:verb", true},
{"MultipleLiteralsWithVerb", "/v1/books:verb", true},
{"SingleWildcard", "/v1/*", true},
{"DoubleWildcard", "/v1/**", true},
{"SingleWildcardWithVerb", "/v1/*:verb", true},
{"DoubleWildcardWithVerb", "/v1/**:verb", true},
{"SingleWildcardFollowedByLiteral", "/v1/*/books", true},
{"DoubleWildcardFollowedByLiteral", "/v1/**/books", true},
{"LiteralFollowedBySingleWildcard", "/v1/books/*", true},
{"LiteralFollowedByDoubleWildcard", "/v1/books/**", true},
{"VariableWithFieldpath", "/v1/{field}", true},
{"VariableWithNestedFieldpath", "/v1/{field.subfield}", true},
{"VariableWithUltraNestedFieldpath", "/v1/{field.subfield.subsubfield}", true},
{"VariableWithLiteralTemplate", "/v1/{field=books}", true},
{"VariableWithSingleWildcardTemplate", "/v1/{field=*}", true},
{"VariableWithDoubleWildcardTemplate", "/v1/{field=**}", true},
{"VariableWithSingleWildcardFollowedByLiteral", "/v1/{field=*/books}", true},
{"VariableWithDoubleWildcardFollowedByLiteral", "/v1/{field=**/books}", true},
{"VariableWithLiteralFollowedBySingleWildcard", "/v1/{field=books/*}", true},
{"VariableWithLiteralFollowedByDoubleWildcard", "/v1/{field=books/**}", true},
{"VariableFollowedByLiteral", "/v1/{field}/books", true},
{"VariableFollowedByVariable", "/v1/{field}/{otherField}", true},
{"VariableWithTemplateFollowedByLiteral", "/v1/{field=books/*}/shelves", true},
{"VariableFollowedByVariableWithTemplate", "/v1/{field}/{otherField=books/*}", true},
{"VariableWithTemplateFollowedByVariableWithTemplate", "/v1/{field=books/*}/{otherField=shelves/*}", true},

// Invalid cases
{"LiteralWithoutLeadingSlash", "v1", false},
{"LiteralFollowedBySlash", "/v1/", false},
{"WrongVerbDelimiter", "/v1-verb", false},
{"MultipleVerbs", "/v1:verb:verb", false},
{"VerbFollowedBySlash", "/v1:verb/", false},
{"MultipleLiteralsWithWrongDelimiter", "/v1|books", false},
{"SingleWildcardFollowedBySlash", "/v1/*/", false},
{"DoubleWildcardFollowedBySlash", "/v1/**/", false},
{"TripleWildcard", "/v1/***", false},
{"WrongVariableMarker", "/v1/[field]", false},
{"WrongVariableSubfieldOperator", "/v1/[field->subfield]", false},
{"VariableTemplateContainsVariable", "/v1/{field={otherField=*}}", false},
{"WrongVariableTemplateAssignmentOperator", "/v1/{field≈books}", false},
}
for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
import "google/api/annotations.proto";
service Library {
rpc FooMethod(FooMethodRequest) returns (FooMethodResponse) {
option (google.api.http) = {
get: "{{.URI}}"
};
}
}
message FooMethodRequest {}
message FooMethodResponse {}
`, test)

problems := httpTemplateSyntax.Lint(file)

if test.valid && len(problems) > 0 {
t.Fatalf("Expected valid HTTP path template syntax but got invalid")
}

if !test.valid && len(problems) == 0 {
t.Fatalf("Expected invalid HTTP path template syntax but got valid")
}
})
}
}

0 comments on commit e55a765

Please sign in to comment.