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

Standard method checks should allow out-of-package messages #465

Closed
jgeewax opened this issue Jan 20, 2020 · 4 comments
Closed

Standard method checks should allow out-of-package messages #465

jgeewax opened this issue Jan 20, 2020 · 4 comments
Assignees

Comments

@jgeewax
Copy link
Contributor

jgeewax commented Jan 20, 2020

For example

rpc CreateBook(CreateBookRequest) returns (other.package.Book);

This should be allowed (I think), but am not sure if it's covered.

@lukesneeringer
Copy link
Contributor

This actually would be allowed now (we only check the name, not the fully qualified name), although I actually wonder whether it should be disallowed.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jan 23, 2020

I'd argue that it's generally bad practice to use other APIs protos in your RPCs input/output types, however "the presence of dots" does not necessarily mean "it belongs to another API" (it might be owned by the same group, etc).

I'll look for a test case for this just to be sure. If there isn't one, we should add one.

In future, we should come up with a different rule that says "Don't copy and use other people's proto files unless you control them or they're considered 'sealed'" and have a lint check for that.

@lukesneeringer
Copy link
Contributor

Sure, but this bug is specifically about the standard methods. And I really can not think of a reason why standard methods would use protos from different packages (other than g.p.Empty, which we explicitly permit) that does not violate our standards.

@lukesneeringer
Copy link
Contributor

This is not coming up in user reports, and we do not agree on whether it should be allowed (it currently is), so going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants