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.

Sign up for GitHub

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

Jump to bottom

fix(spring): update hasRestOption to exclude services with no rest-supported methods #1343

Merged
merged 1 commit into from
Feb 9, 2023
Merged

fix(spring): update hasRestOption to exclude services with no rest-supported methods #1343

merged 1 commit into from
Feb 9, 2023

Conversation

emmileaf
Copy link
Contributor

@emmileaf emmileaf commented Feb 9, 2023

This PR aligns the spring generator with main branch changes from #1117. I copied the relevant isSupportedByTransport(Transport.REST) logic as a quick fix instead of cherry-picking the commits, since the branches have diverged a bit since the monorepo migration.

Future note: as part of migrating this spring generator code away from its separate branch and depending on the gapic generator jar, I'd like to revisit this logic to call the corresponding helpers in gapic-generator-java main.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Feb 9, 2023
@emmileaf emmileaf added the spring pr that's related to spring code gen, intend to merge into autoconfig-gen-draft2 branch. label Feb 9, 2023
@emmileaf emmileaf requested review from blakeli0 and lqiu96 February 9, 2023 19:19
@emmileaf emmileaf marked this pull request as ready for review February 9, 2023 19:20
@emmileaf emmileaf requested a review from a team as a code owner February 9, 2023 19:20
@sonarcloud
Copy link

sonarcloud bot commented Feb 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

76.5% 76.5% Coverage
0.0% 0.0% Duplication

blakeli0
blakeli0 approved these changes Feb 9, 2023
View reviewed changes
Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's try to get this in first and unblock the release, can you please add a test for it in a separate PR? So that we know this functionality is not breaking when we do the migration.

@emmileaf
Copy link
Contributor Author

emmileaf commented Feb 9, 2023

LGTM. Let's try to get this in first and unblock the release, can you please add a test for it in a separate PR? So that we know this functionality is not breaking when we do the migration.

Sounds good, will merge and follow-up on this since the existing golden tests don’t cover for the additional REST-related logic.

@lqiu96 I saw that you had added wicked.proto as part of your changes, and was wondering if you could help me understand additional context on this test service?

@emmileaf emmileaf merged commit 3f5db4d into autoconfig-gen-draft2 Feb 9, 2023
@emmileaf emmileaf deleted the spring-rest-patch branch February 9, 2023 19:36
@lqiu96
Copy link
Contributor

lqiu96 commented Feb 9, 2023 •
edited
Loading

LGTM. Let's try to get this in first and unblock the release, can you please add a test for it in a separate PR? So that we know this functionality is not breaking when we do the migration.

Sounds good, will merge and follow-up on this since the existing golden tests don’t cover for the additional REST-related logic.

@lqiu96 I saw that you had added wicked.proto as part of your changes, and was wondering if you could help me understand additional context on this test service?

The wicked.proto was intended to be part of the showcase tests to cover the new REST method generation logic (i.e. a generic proto that had non-REST enabled or eligible RPCs in a service). Showcase testing is primarily used to cover the functionality of a generated client library's behavior (i.e. having the generated library hit a mock server and validating the behavior) whereas this proto was primarily used to test the generator's generation behavior. There was some pushback so there is an "extended" framework called gapic-showcase-extended where I've put this proto. The extended showcase module is able to test additional stuff like generator's generation behavior + anything else we may need in the future.

@emmileaf
Copy link
Contributor Author

emmileaf commented Feb 9, 2023

@lqiu96 Gotcha, thanks for the explanation. I think I'll try to re-use as much as this unit testing setup as I can for testing generation behaviour on the spring side as well. Really appreciate all the work you've already laid out here!

@lqiu96
Copy link
Contributor

lqiu96 commented Feb 9, 2023

@emmileaf Sounds good! There may be a few lingering issues with the showcase-extended module. Let me know and I can try to help resolve them

This was referenced Feb 9, 2023
Merged
Merged
emmileaf added a commit that referenced this pull request Feb 16, 2023
@emmileaf
fix(spring): extend hasRestOption fix to properties composer and add …
dcd0823 
…golden tests (#1348)

This PR is a follow-up on #1343:
 - Extends fix to SpringPropertiesClassComposer, so that for services without REST-enabled rpcs, the unused useRest property is also not generated
- Adds golden tests for the updated hasRestOption scenario using the wicked proto
- Updates SpringAutoconfigCommentComposer for javadoc comments alluding to the useRest option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqiu96 lqiu96 lqiu96 approved these changes

@blakeli0 blakeli0 blakeli0 approved these changes

Assignees
No one assigned
Labels
size: s Pull request size is small. spring pr that's related to spring code gen, intend to merge into autoconfig-gen-draft2 branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emmileaf @lqiu96 @blakeli0