test: add a baseline test that covers possible naming conflicts #668
Conversation
google-cla
bot
added
the
cla: yes
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| initialize( |
There was a problem hiding this comment.
oops! we already have initialize above
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| servicePath( |
There was a problem hiding this comment.
oops! we already have servicePath above
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| apiEndpoint( |
There was a problem hiding this comment.
oops! we already have apiEndpoint above
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| port( |
There was a problem hiding this comment.
oops! we already have port above
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| scopes( |
There was a problem hiding this comment.
oops! we already have scopes above
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| getProjectId( |
There was a problem hiding this comment.
oops! we already have getProjectId above
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| function( |
There was a problem hiding this comment.
Just oops
| * The first element of the array is an object representing [Empty]{@link google.protobuf.Empty}. | ||
| * The promise has a method named "cancel" which cancels the ongoing API call. | ||
| */ | ||
| constructor( |
There was a problem hiding this comment.
Big ooops
| * console.log(decodedOperation.metadata); | ||
| * | ||
| */ | ||
| async checkLongRunningProgress(name: string): Promise<LROperation<protos.google.protobuf.Empty, protos.google.protobuf.Empty>>{ |
There was a problem hiding this comment.
Oops! We already has this one.
alexander-fenster
force-pushed
the
name-conflict-test
branch
from
206e787 to
709d777
Compare
| * @returns {Stream} | ||
| * An object stream which emits an object representing [Empty]{@link google.protobuf.Empty} on 'data' event. | ||
| */ | ||
| paginatedMethodStream( |
There was a problem hiding this comment.
Name conflict again.
| * @returns {Object} | ||
| * An iterable Object that conforms to @link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols. | ||
| */ | ||
| paginatedMethodAsync( |
There was a problem hiding this comment.
Name conflict as well.
What are we really trying to achieve with such a proto? I see in your description you're going to check this in once all of the naming issues are resolved. Do you plan on fixing all such collisions? What if some names should not be used ever? We can probably make some guidance for that (e.g. "never name an RPC 'Public'" for java or something like that). |
There was a problem hiding this comment.
LGTM with some thoughts:
checkMethodProgressfor LRO methods, is it doing the same thing as our addedcheckMethodProgresshelper? If it is, we probably wanna replace it, if not, we can change to another name for avoiding conflict.- when the rpc name hits the reserved words like
function, should we throw error for them during generation? Otherwise, if we put this naming.proto to gapic-showcase, we probably wanna add more reserved words for other languanges likeclass, enum, etc. @noahdietz
Thank you!
|
@xiaozhenliu-gg5 re: @xiaozhenliu-gg5 @noahdietz re: "bad names", unfortunately, the corresponding AIP is not strict enough - AIP-140 only suggests that field names should not use words that might be reserved, and there is no guidance on method names. So all examples above are perfectly valid from AIP point of view. What I'm planning to do with this is to introduce a simple method renaming logic in the generator - basically, the generator should know of the reserved names (all RPC names and language keywords) and it should not add conflicting names to the class. I expect to make the generator finally generate a working library based on this bad proto (which I consider to be AIP-compliant), but this is not the main goal here. The main goal is to have a test coverage for this planned refactor. Before I can start working on this refactor, I must have a baseline test checked in, because it's the only way to cover the refactored code with tests (since all our existing tests do not have naming conflicts, the name resolution logic will never trigger in the regular test coverage). Hence this new baseline. I started this because of an Ads method that triggered the naming conflict similar to #1 above, and with this baseline, the future change to |
…rator-typescript into name-conflict-test
alexander-fenster
force-pushed
the
name-conflict-test
branch
from
2f6d0b3 to
3fe89c2
Compare
This PR adds a baseline test for one Very Bad Proto. This proto is so bad that the generated library will never compile (I left some comments below to show the problems).
We have some naming problems in the generator that do not affect any existing API so far, but one of those problem (the
*Streamone, see below) affects Ads APIs and will backfire when we start generating Ads TypeScript client library.Currently, the client library class has some extra stuff that can have name conflicts with protobuf methods:
For any paginated method
method, we generate extra functionsmethodStreamandmethodAsync, both can conflict with an actual RPC with the same name. (this is what happens with Ads protos - they have a paginatedrpc Somethingand alsorpc SomethingStream)For any long running method
method, we generate an extra functioncheckMethodProgresswhich can conflict with an actual RPC with the same name.We have some other methods in our class (e.g.
initializeandgetProjectId) which will conflict with RPCs with the same names.An RPC can be named in a way that won't work in TypeScript (e.g.
rpc Constructororrpc Function). This is discouraged by AIPs but possible.As a result, the generator works perfectly in these cases, but the resulting code cannot compile.
This proto does not currently handle all bad use cases (e.g. the one in #609 is not covered) but I will update it with more naming conflict findings.
These problems are going to be solved by some naming refactor (I'm still thinking about the proper design), and when the code for the
namingbaseline finally compiles, I'll add its compilation to CircleCI to make we don't break things after this is fixed.Cc: @noahdietz - as we discussed in the chat, it might be only TypeScript problem for now, but I guess we still can have problems with reserved words in other languages. Do you think making this kind of "bad bad proto" (covering all languages and not just TypeScript) would make sense as a team effort?