search

googleapis / api-linter

A linter for APIs defined in protocol buffers.
https://linter.aip.dev/
Apache License 2.0
593 stars 144 forks source link

Issues running api-linter on googleapis/googleapis #306

Closed bufdev closed 5 years ago

bufdev commented 5 years ago

This is using latest commit https://github.com/googleapis/api-linter/commit/b7c8cacd5574838c908c88ad5c472258156aaba7

$ git remote -v
origin  https://github.com/googleapis/googleapis (fetch)
origin  https://github.com/googleapis/googleapis (push)

$ git log -n 1
commit 698b05fe2b05893dfe74a156a5665f879de7fceb (HEAD -> master, origin/master, origin/HEAD)
Author: Google APIs <noreply@google.com>
Date:   Wed Oct 23 08:10:26 2019 -0700

    Synchronize new proto/yaml changes.

    PiperOrigin-RevId: 276276483

$ api-linter $(find . -name '*.proto')
2019/11/05 16:41:01 google/ads/googleads/v1/resources/ad.proto:42:1: duplicate symbol google.ads.googleads.v1.resources.Ad: already defined as message in "./google/ads/googleads/v1/resources/ad.proto"

# removing the leading './' from find output
$ api-linter $(find . -name '*.proto' | sed 's/^\.\///')
2019/11/05 16:41:12 runtime error: invalid memory address or nil pointer dereference

# prints something with relative path for one file and exits with code 0
$ api-linter google/type/date.proto
- file_path: google/type/date.proto
  problems: []

# change the above to absolute path
$ api-linter $(pwd)/google/type/date.proto
2019/11/05 16:57:16 /Users/foo/googleapis/google/type/date.proto: open Users/foo/googleapis/google/type/date.proto: no such file or directory
mingzhi commented 5 years ago

About the absolute path issue: Like protoc, the proto parser we are using only works with relative paths. So proto files with absolute paths are not supported.

About the runtime error issue: It is due to a problem of getting SourceCodeInfo for a map field. In the compiled file descriptor, a map field will become a message, e.g., in google/cloud/iot/v1/resources.proto

map<string, string> metadata = 17;

will become

message MetadataEntry {
  string key = 1;
  string value = 2;
}

repeated MetadataEntry metadata = 17;

As a result, we have troubles getting SourceCodeInfo for the map field, since MetadataEntry is no where in the original proto file.

bufdev commented 5 years ago

About the absolute path issue: Like protoc, the proto parser we are using only works with relative paths. So proto files with absolute paths are not supported.

Protoreflect works fine with absolute paths if you use ResolveFilenames, I added it for this purpose https://github.com/jhump/protoreflect/pull/199

About the runtime error issue: It is due to a problem of getting SourceCodeInfo for a map field. In the compiled file descriptor, a map field will become a message, e.g., in google/cloud/iot/v1/resources.proto

map<string, string> metadata = 17;

will become

message MetadataEntry {
  string key = 1;
  string value = 2;
}

repeated MetadataEntry metadata = 17;

As a result, we have troubles getting SourceCodeInfo for the map field, since MetadataEntry is no where in the original proto file.

Any lint rule should handle this (I think that's semi-obvious though).

I think the central point here is that api-linter should work out of the box for googleapis - it was the first test I ran. The big issue is the usage of protoreflect - you need to clean up file paths before passing them to the Parser. Glancing at the code, there's issues around the protoreflect usage, but this is the major one I see.

mingzhi commented 5 years ago

Agree! Please report any more issues you found. We will address the two issues reported here. Thanks a lot!

bufdev commented 5 years ago

Sounds good - a quick note is that Buf uses protoreflect as well and all of the above are handled (namely any combination of relative/absolute paths, unlike protoc), so it IS possible when using protoreflect, you just need to clean up file paths (and proto_paths) carefully.

bufdev commented 5 years ago

I haven't looked into it carefully, so it might be OK, but this logic will probably cause similar issues https://github.com/googleapis/api-linter/blob/a1d8d13a9ea4eab824cc1dc9cbee7e50da352c05/cmd/api-linter/cli.go#L118

bufdev commented 5 years ago

(I think this was a mistaken auto close, but just in case it’s missed, this wasn’t fixed by #309 FYI @mingzhi)

mingzhi commented 5 years ago

I verified that the runtime-error issue is fixed:

$ cd ~/Projects/googleapis/googleapis
$ api-linter --output-format summary $(find . -name '*.proto' | sed 's/^\.\///')
+--------------------------------------------+------------------+----------------+
|                    RULE                    | TOTAL VIOLATIONS | VIOLATED FILES |
+--------------------------------------------+------------------+----------------+
| core::0131::http-body                      |                1 |              1 |
| core::0131::http-method                    |                1 |              1 |
| core::0151::lro-metadata-type              |                2 |              2 |
| core::0151::lro-types-defined-in-file      |                3 |              1 |
| core::0133::http-method                    |                3 |              1 |
| core::0136::http-method                    |                4 |              4 |
| core::0134::request-message-name           |                5 |              4 |
| core::0133::request-message-name           |                7 |              5 |
| core::0135::response-message-name          |                9 |              6 |
| core::0203::input-only                     |                9 |              5 |
| core::0131::synonyms                       |               11 |              9 |
| core::0136::http-body                      |               15 |             15 |
| core::0136::prepositions                   |               15 |              6 |
| core::0141::count-suffix                   |               17 |             12 |
| core::0151::lro-response-type              |               19 |             13 |
| core::0131::response-message-name          |               20 |             15 |
| core::0133::response-message-name          |               20 |             16 |
| core::0134::response-message-name          |               21 |             14 |
| core::0133::request-resource-field         |               25 |             22 |
| core::0134::request--resource-field        |               26 |             19 |
| core::0140::abbreviations                  |               27 |              9 |
| core::0140::base64                         |               28 |             17 |
| core::0141::forbidden-types                |               29 |             19 |
| core::0135::request-name-field             |               31 |             25 |
| core::0135::http-uri-name                  |               32 |             25 |
| core::0134::request-mask-field             |               34 |             25 |
| core::0142::time-field-names               |               34 |             27 |
| core::0134::http-method                    |               35 |             25 |
| core::0133::request-parent-field           |               39 |             29 |
| core::0158::response-next-page-token-field |               39 |             28 |
| core::0158::request-page-token-field       |               40 |             28 |
| core::0134::http-body                      |               42 |             30 |
| core::0143::standardized-codes             |               43 |             22 |
| core::0158::request-page-size-field        |               46 |             31 |
| core::0203::immutable                      |               49 |             26 |
| core::0136::verb-noun                      |               51 |             24 |
| core::0133::http-body                      |               54 |             38 |
| core::0134::synonyms                       |               65 |             30 |
| core::0143::string-type                    |               65 |             40 |
| core::0134::http-uri-name                  |               66 |             43 |
| core::0135::request-unknown-fields         |               75 |             36 |
| core::0122::camel-case-uris                |               85 |             11 |
| core::0132::request-parent-field           |               87 |             63 |
| core::0133::request-unknown-fields         |              110 |             45 |
| core::0151::operation-info                 |              110 |             41 |
| core::0134::request-unknown-fields         |              157 |             53 |
| core::0133::http-uri-parent                |              176 |            108 |
| core::0142::time-field-type                |              189 |             73 |
| core::0132::request-unknown-fields         |              212 |             79 |
| core::0131::request-name-field             |              241 |            227 |
| core::0131::http-uri-name                  |              257 |            235 |
| core::0140::prepositions                   |              281 |            111 |
| core::0131::request-unknown-fields         |              339 |            248 |
| core::0136::http-uri-suffix                |              391 |            205 |
| core::0203::output-only                    |              820 |            140 |
| core::0192::only-leading-comments          |             1036 |           1036 |
| core::0203::required                       |             1060 |            256 |
| core::0192::has-comments                   |             1182 |            164 |
+--------------------------------------------+------------------+----------------+
Linted 1604 proto files
mingzhi commented 5 years ago

Sent out PR #313 for a quick fix on the issue about absolute paths.

bufdev commented 5 years ago

I think this is fixed, thanks for getting it in :-)