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")); |
| Method uploadMethod = methods.get(0); | ||
| assertEquals("UploadMedia", uploadMethod.name()); | ||
| assertFalse(uploadMethod.isResumableUpload()); |
There was a problem hiding this comment.
Since we added the showcase pattern to the allowlist, we can now assert that isResumableUpload() is indeed true for the UploadMedia method, which properly tests the parser's allowlist matching logic.
| Method uploadMethod = methods.get(0); | |
| assertEquals("UploadMedia", uploadMethod.name()); | |
| assertFalse(uploadMethod.isResumableUpload()); | |
| Method uploadMethod = methods.get(0); | |
| assertEquals("UploadMedia", uploadMethod.name()); | |
| assertTrue(uploadMethod.isResumableUpload()); |
| 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()); | ||
| } |
There was a problem hiding this comment.
With the showcase pattern added to the parser's allowlist, the parser will automatically identify UploadMedia as a resumable upload. Therefore, the manual workaround adaptShowcaseResumableUploadForTest is no longer needed and can be removed entirely.
| 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()); | |
| } | |
| return GapicContext.builder() | |
| .setMessages(messageTypes) | |
| .setResourceNames(resourceNames) | |
| .setServices(services) | |
| .setHelperResourceNames(outputResourceNames) | |
| .setTransport(transport) | |
| .setServiceConfig(GapicServiceConfig.create(Optional.empty())) | |
| .build(); | |
| } |
e7acfcd to
aa32171
Compare
|
|
aa32171 to
681c369
Compare
1 participant
Work in progress, not ready for review