feat(generator): add model flag and allowlist parser for resumable upload RPCs - #14317
feat(generator): add model flag and allowlist parser for resumable upload RPCs#14317whowes wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for identifying resumable upload methods in the GAPIC generator by adding an isResumableUpload property to the Method model and updating the Parser to match RPC names against an allowlist. The review feedback suggests populating the RESUMABLE_UPLOAD_ALLOWLIST_PATTERNS with the showcase service's pattern to make the parser logic functional and testable, which would also allow the removal of a manual workaround in TestProtoLoader and enable a proper assertion in ParserTest.
| private static final ImmutableList<Pattern> RESUMABLE_UPLOAD_ALLOWLIST_PATTERNS = | ||
| ImmutableList.of(); |
There was a problem hiding this comment.
The RESUMABLE_UPLOAD_ALLOWLIST_PATTERNS list is currently empty, which means no RPCs will ever be identified as resumable uploads by the parser. To make this functional and testable, we should add the showcase service's pattern to this list. This also allows us to write a proper assertion in ParserTest and avoid the manual workaround in TestProtoLoader.
| private static final ImmutableList<Pattern> RESUMABLE_UPLOAD_ALLOWLIST_PATTERNS = | |
| ImmutableList.of(); | |
| private static final ImmutableList<Pattern> RESUMABLE_UPLOAD_ALLOWLIST_PATTERNS = | |
| ImmutableList.of( | |
| Pattern.compile("google\\.showcase\\.v1beta1\\.ResumableUploadService\\.UploadMedia")); |
There was a problem hiding this comment.
This omission is intentional - allowlist will be populated when all composers are complete, unit tests use a different mechanism to enable for now.
There was a problem hiding this comment.
Do we have to wait until composers are complete? Is it to prevent accidentally generation? I think we can at least add the showcase methods here.
There was a problem hiding this comment.
My main motivation here is so that we can generate the showcase ResumableUploadService client library cleanly without resumable upload support (#14324) followed immediately by the regeneration with support added via being allowlisted (#14325). IMO the diff on the latter PR gives a clear rollup overview of the resumable upload changes across the client library that's much easier to review than if the whole generation was bundled together, and adds value that we don't get from just the diffs we see in the individual composer PRs.
I originally tried to do the baseline generation earlier followed by allowlisting (before the composer changes) but I couldn't get it to work across intermediate PRs with both verify.sh working AND the generated library compiling. So keeping the allowlist empty until all composers are in place is was what I landed on.
| Method uploadMethod = methods.get(0); | ||
| assertEquals("UploadMedia", uploadMethod.name()); | ||
| assertFalse(uploadMethod.isResumableUpload()); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return GapicContext.builder() | ||
| .setMessages(messageTypes) | ||
| .setResourceNames(resourceNames) | ||
| .setServices(adaptShowcaseResumableUploadForTest(services)) | ||
| .setHelperResourceNames(outputResourceNames) | ||
| .setTransport(transport) | ||
| .setServiceConfig(GapicServiceConfig.create(Optional.empty())) | ||
| .build(); | ||
| } | ||
|
|
||
| private static List<Service> adaptShowcaseResumableUploadForTest(List<Service> services) { | ||
| return services.stream() | ||
| .map( | ||
| s -> | ||
| s.toBuilder() | ||
| .setMethods( | ||
| s.methods().stream() | ||
| .map( | ||
| m -> | ||
| m.name().equals("UploadMedia") | ||
| ? m.toBuilder().setIsResumableUpload(true).build() | ||
| : m) | ||
| .collect(Collectors.toList())) | ||
| .build()) | ||
| .collect(Collectors.toList()); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
e7acfcd to
aa32171
Compare
aa32171 to
681c369
Compare
dbc1403 to
c7dc7e4
Compare
c7dc7e4 to
8351695
Compare
8351695 to
9b6b8b4
Compare
9b6b8b4 to
7f9c11a
Compare
7f9c11a to
0447951
Compare
|
|
2 participants
Adds the
isResumableUpload()property to the generator'sMethodmodel and wires pattern matching inParser.javaagainst an initially empty allowlist. The showcase test proto is also added with a hermetic test adapter to enable isolated unit testing across subsequent composers.