Fix our proto code and swagger generation. #2167
Conversation
…to 0.15.1 (from 0.14.0). Clean up the proto-related targets to match the SDK's.
…d warnings from protoc-swagger.gen about stuff not in the config.
…er for third-party purposes.
WalkthroughThe changes in this pull request focus on restoring the Protobuf generation functionality and transitioning from Swagger to OpenAPI v2 for API documentation. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
SpicyLemon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
proto/buf.gen.swagger.yaml (1)
Verified Removal of
strategy: allOptionThe
strategy: alloption has been successfully removed from theproto/buf.gen.swagger.yamlfile as shown in the latest changes.Clarification Needed for
json_names_for_fields=falsePlease provide more details on the purpose and impact of the
json_names_for_fields=falseoption to ensure it aligns with our project's requirements.🔗 Analysis chain
Line range hint
1-11: Summary of OpenAPI v2 transitionThe changes in this file successfully transition the API documentation generation from Swagger to OpenAPI v2, addressing the main objective of the PR. The plugin name and options have been appropriately updated to reflect this change.
However, there are a couple of points that require attention:
- The new option
json_names_for_fields=falseneeds clarification (as mentioned in the previous comment).- The AI summary mentions the removal of the
strategy: alloption, but this change is not visible in the provided code snippet.To ensure all changes are accounted for, please run the following command to display the full diff of this file:
This will help verify if there are any additional modifications not visible in the current snippet, such as the removal of the
strategy: alloption.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
git diff origin/main -- proto/buf.gen.swagger.yamlLength of output: 553
scripts/protoc-swagger-gen.sh (5)
6-11: Improved path management with centralized variable declarationsThe introduction of standardized variables for directory and file paths enhances readability and maintainability. This change makes it easier to modify paths in the future if needed.
Consider using double quotes for all variable assignments to ensure consistency and handle potential spaces in paths. For example:
-proto_dir='./proto' +proto_dir="./proto"
16-20: Improved proto file processingThe updated logic for finding and processing proto files enhances reliability and efficiency:
- Using
findwith-type fand-print0improves handling of filenames with spaces.- Filtering for files with service definitions prevents processing unnecessary files.
- The simplified loop structure improves readability.
Consider adding a check to ensure
proto_filesis not empty before proceeding with the loop:if [ -z "$proto_files" ]; then echo "No proto files with service definitions found." exit 1 fiThis would provide better error handling if no suitable proto files are found.
24-30: Enhanced feedback with warnings for unreferenced filesThe new section that outputs warnings for generated files not referenced in the config file is a valuable addition. It improves the script's feedback mechanism and helps identify potential issues where generated files are not being included in the final output.
Consider adding a counter for the number of warnings and display a summary at the end:
warning_count=0 for file in $swagger_files; do if ! grep -Fq "\"url\": \"$file\"" "$config_file"; then printf '\033[93mWARNING\033[0m: "%s" not referenced in %s\n' "$file" "$config_file" ((warning_count++)) fi done if [ $warning_count -gt 0 ]; then echo "Total warnings: $warning_count" fiThis would provide a quick overview of how many files were not referenced.
42-45: Improved cleanup and finalization processThe changes in this section enhance the script's consistency and readability:
- Using
-efor multiple patterns in grep improves clarity.- Directing output to
$output_filemaintains consistency with variable declarations.- Using
$VERBOSEfor the cleanup message aligns with the rest of the script.Consider adding a final success message when the script completes without errors:
echo "Swagger generation completed successfully. Output file: $output_file"This would provide clear confirmation that the script has finished its task as expected.
1-45: Overall improvements align well with PR objectivesThis script has undergone significant enhancements that align well with the PR objectives and the requirements outlined in the linked issue #2135:
- The script now uses standardized variables for paths and flags, improving maintainability.
- Proto file processing has been optimized to handle only relevant files with service definitions.
- The addition of warnings for unreferenced generated files enhances the feedback mechanism.
- The use of
swagger-combinewith the updated config file supports the transition to OpenAPI v2.- The cleanup process has been improved for consistency and clarity.
These changes contribute to resolving the issues with proto generation and support the transition from Swagger to OpenAPI v2. The script is now more robust and easier to maintain, which should help address the problems mentioned in issue #2135.
To further improve the script's robustness and alignment with best practices, consider:
- Adding error handling for critical operations (e.g., directory creation, file operations).
- Implementing a
--helpoption to display usage information.- Using a cleanup trap to ensure temporary files are removed even if the script exits unexpectedly.
Example:
trap 'rm -rf "$temp_dir"' EXITThese additions would make the script more resilient and user-friendly.
client/docs/config.json (3)
244-256: LGTM! Consider adding a comment for clarity.The addition of the IBC Hooks API entry is well-structured and consistent with other entries. It aligns with the PR objective to include ibchooks queries in the Swagger documentation.
Consider adding a comment above this entry to explain its purpose and any specific considerations for the IBC Hooks API. This would improve maintainability and provide context for future developers.
+ // IBC Hooks API - Added to support queries related to IBC hooks functionality { "url": "./tmp-swagger-gen/provenance/ibchooks/v1/query.swagger.json", "tags": { "add": [ "IBC Hooks" ] }, "operationIds": { "rename": { "Params": "IBCHooksParams" } } },
Line range hint
1-243: LGTM! Consider standardizing tag naming convention.The changes to existing entries, including the renaming of operation IDs and the addition of tags, improve the clarity and organization of the Swagger documentation. These modifications align well with the PR objectives.
For improved consistency, consider standardizing the tag naming convention. Most tags are singular (e.g., "Attribute", "Exchange"), but "IBC Hooks" is plural. Consider changing it to singular form:
{ "url": "./tmp-swagger-gen/provenance/ibchooks/v1/query.swagger.json", "tags": { "add": [ - "IBC Hooks" + "IBC Hook" ] }, "operationIds": { "rename": { "Params": "IBCHooksParams" } } },This change would maintain consistency across all API tags in the configuration.
Also applies to: 257-283
Action Required: Missing Transaction APIs and Inconsistent Operation IDs
The verification process identified several issues that need attention:
Missing Transaction APIs:
holdtransaction API is missingsanctionquery and transaction APIs are missingquarantinequery and transaction APIs are missingInconsistent Operation ID Renaming:
- Some
'Params'operation IDs may not have been renamedPlease address these issues to ensure the Swagger configuration is complete and consistent.
🔗 Analysis chain
Line range hint
1-283: Overall, excellent improvements to Swagger configuration.The changes in this file significantly contribute to the PR objectives of fixing proto generation and improving Swagger documentation. The addition of the IBC Hooks API, renaming of operation IDs, and categorization of APIs with tags enhance the clarity and organization of the Swagger documentation.
A few minor suggestions have been made for further improvement:
- Adding a comment for the new IBC Hooks API entry.
- Standardizing the tag naming convention.
These changes align well with the transition from Swagger to OpenAPI v2 and the overall cleanup of proto-related configurations.
To ensure all necessary APIs are included and properly configured, please run the following verification script:
This script will help ensure that all expected APIs are included, operation IDs are consistently renamed, and tags follow a singular naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness and consistency of API entries in the Swagger configuration. # Test 1: Check if all expected API modules are present expected_modules=("attribute" "exchange" "hold" "ibchooks" "ibcratelimit" "marker" "metadata" "msgfees" "name" "oracle" "sanction" "quarantine" "trigger") for module in "${expected_modules[@]}"; do if ! grep -q "provenance/$module/v1/query.swagger.json" client/docs/config.json; then echo "Warning: $module query API is missing" fi if ! grep -q "provenance/$module/v1/tx.swagger.json" client/docs/config.json; then echo "Warning: $module transaction API is missing" fi done # Test 2: Verify consistent operation ID renaming if grep -q '"Params":' client/docs/config.json; then echo "Warning: Some 'Params' operation IDs may not have been renamed" fi # Test 3: Check for consistent tag naming (singular form) plural_tags=$(grep -oP '"add": \[\K"[^"]+ [^"]+"' client/docs/config.json) if [ ! -z "$plural_tags" ]; then echo "Warning: The following tags may be in plural form: $plural_tags" fi echo "Verification complete. Please review any warnings."Length of output: 3479
Makefile (3)
489-492: Simplified proto-gen target with improved modularityThe proto-gen target has been streamlined, which aligns with the PR objective of cleaning up proto-related Makefile targets. The use of a single Docker command simplifies the process and reduces potential points of failure.
Consider extracting the go.mod backup and restore commands into a separate function or target for better modularity and reusability. This could be useful if other targets need similar functionality in the future.
define backup-restore-go-mod cp go.mod .go.mod.bak $(1) mv .go.mod.bak go.mod $(GO) mod tidy endef proto-gen: @echo "Generating Protobuf files" $(call backup-restore-go-mod,$(protoImage) sh ./scripts/protocgen.sh)
498-517: Streamlined proto-related targetsThe proto-format, proto-lint, proto-check-breaking, and proto-check-breaking-third-party targets have been simplified, which aligns with the PR objective of cleaning up proto-related Makefile targets. The use of a single Docker command for each target simplifies the process and reduces potential points of failure.
For consistency and improved readability, consider using multi-line commands for the proto-lint, proto-check-breaking, and proto-check-breaking-third-party targets, similar to the proto-format target. This would make the Makefile more uniform and easier to maintain. For example:
proto-lint: @echo "Linting Protobuf files" $(protoImage) buf lint \ --error-format=json proto-check-breaking: @echo "Check breaking Protobuf files" $(protoImage) buf breaking proto \ --against '$(HTTPS_GIT)#branch=main,subdir=proto' \ --error-format=json proto-check-breaking-third-party: @echo "Check breaking 3rd party Protobuf files" $(protoImage) buf breaking third_party/proto \ --against '$(HTTPS_GIT)#branch=main,subdir=third_party/proto' \ --error-format=json
480-517: Overall improvements in proto-related Makefile targetsThe changes made to the Makefile, particularly in the proto-related targets, significantly improve the overall structure and maintainability of the build process. Key improvements include:
- Updating the proto builder version to 0.15.1.
- Simplifying variable names and reducing redundancy.
- Streamlining Docker commands for proto generation, formatting, linting, and breaking changes checks.
- Improving the handling of go.mod during proto generation to prevent unintended changes.
These changes align well with the PR objectives of cleaning up proto-related Makefile targets, upgrading the proto image version, and transitioning to OpenAPI v2.
To further improve the Makefile's maintainability and flexibility:
- Consider creating a separate include file for proto-related targets and variables. This would help keep the main Makefile cleaner and make it easier to manage proto-specific configurations.
- Implement a versioning system for the Makefile itself, which could help track changes over time and make it easier to roll back if needed.
- Add comments explaining the purpose and functionality of each major target, especially for the more complex ones. This would improve the onboarding experience for new contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (25)
x/attribute/types/query.pb.gois excluded by!**/*.pb.gox/attribute/types/tx.pb.gois excluded by!**/*.pb.gox/exchange/query.pb.gois excluded by!**/*.pb.gox/exchange/tx.pb.gois excluded by!**/*.pb.gox/hold/query.pb.gois excluded by!**/*.pb.gox/ibchooks/types/query.pb.gois excluded by!**/*.pb.gox/ibchooks/types/tx.pb.gois excluded by!**/*.pb.gox/ibcratelimit/query.pb.gois excluded by!**/*.pb.gox/ibcratelimit/tx.pb.gois excluded by!**/*.pb.gox/marker/types/query.pb.gois excluded by!**/*.pb.gox/marker/types/tx.pb.gois excluded by!**/*.pb.gox/metadata/types/query.pb.gois excluded by!**/*.pb.gox/metadata/types/tx.pb.gois excluded by!**/*.pb.gox/msgfees/types/query.pb.gois excluded by!**/*.pb.gox/msgfees/types/tx.pb.gois excluded by!**/*.pb.gox/name/types/query.pb.gois excluded by!**/*.pb.gox/name/types/tx.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gois excluded by!**/*.pb.gox/oracle/types/tx.pb.gois excluded by!**/*.pb.gox/quarantine/query.pb.gois excluded by!**/*.pb.gox/quarantine/tx.pb.gois excluded by!**/*.pb.gox/sanction/query.pb.gois excluded by!**/*.pb.gox/sanction/tx.pb.gois excluded by!**/*.pb.gox/trigger/types/query.pb.gois excluded by!**/*.pb.gox/trigger/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
- .changelog/unreleased/bug-fixes/2135-openapiv2.md (1 hunks)
- Makefile (1 hunks)
- client/docs/config.json (1 hunks)
- proto/buf.gen.swagger.yaml (1 hunks)
- scripts/protoc-swagger-gen.sh (1 hunks)
- scripts/protocgen.sh (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changelog/unreleased/bug-fixes/2135-openapiv2.md
🔇 Additional comments (7)
proto/buf.gen.swagger.yaml (3)
4-4: Excellent transition to OpenAPI v2The change from
swaggertoopenapiv2as the plugin name is a crucial step in addressing the PR's main objective. This modification aligns the configuration with the new OpenAPI v2 standard, resolving the compatibility issues mentioned in the linked issue #2135.
8-8: Correct adaptation of fully qualified names optionThe update from
fqn_for_swagger_name=truetofqn_for_openapi_name=trueis a necessary adjustment to maintain the use of fully qualified names in the new OpenAPI v2 format. This change ensures consistency in naming conventions throughout the generated documentation.
10-10: Clarify the purpose of the new JSON names optionA new option
json_names_for_fields=falsehas been added. While this aligns with the overall goal of updating the OpenAPI configuration, it wasn't explicitly mentioned in the PR objectives or linked issue.Could you please clarify:
- The specific reason for adding this option?
- How does this affect the generated OpenAPI documentation?
- Are there any potential impacts on existing integrations or clients that we should be aware of?
This information will help ensure that the change aligns with the project's documentation standards and doesn't introduce unexpected behavior.
scripts/protoc-swagger-gen.sh (3)
3-5: Improved verbose flag handlingThe introduction of the
VERBOSEvariable and support for both short (-v) and long (--verbose) options enhances the script's usability and follows bash conventions.
15-15: Consistent use of temporary directory variableThe use of
$temp_dirfor creating the temporary directory aligns with the new variable declarations, improving consistency and maintainability.
36-36: Improved clarity in swagger file combinationThe use of defined variables
$config_fileand$temp_filein the swagger-combine command enhances clarity and maintains consistency with the rest of the script. This change improves maintainability.Makefile (1)
480-482: Proto builder version updated and variables simplifiedThe proto builder version has been updated from 0.14.0 to 0.15.1, which aligns with the PR objective of upgrading the proto image version. The variable names have been simplified, which improves readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
scripts/protocgen.sh (1)
Line range hint
1-24: Consider adding documentation for the new workflowThe changes to this script align well with the PR objectives of updating proto-related scripts. The removal of the
protoc_gen_gocosmosfunction and the addition of new logic for verbose output and proto file processing improve the script's focus and efficiency.However, it might be beneficial to add some comments or documentation explaining:
- The purpose of using
buf generateinstead of the previous method.- The significance of the
option go_package.*provenancepattern in selecting proto files.- Any prerequisites or setup required for this new workflow.
This additional documentation would help future maintainers understand the rationale behind these changes and how to use or modify the script effectively.
scripts/protoc-swagger-gen.sh (5)
6-11: LGTM: Improved variable declarations for better maintainabilityThe introduction of these variables significantly improves the script's readability and maintainability. Using variables instead of hardcoded paths is a best practice in shell scripting.
Consider adding comments for each variable to explain their purpose, especially for less obvious ones like
temp_fileandtemplate_file. This would further enhance the script's maintainability.
15-20: LGTM: Improved proto file processing logicThe new approach for finding and processing proto files is more robust and efficient. It correctly identifies only the relevant files containing service definitions.
For added robustness, consider quoting the
$filevariable in the loop to handle potential spaces in filenames:for file in $proto_files; do [ -n "$VERBOSE" ] && printf 'Generating swagger file for [%s].\n' "$file" buf generate --template "$template_file" "$file" done
24-31: LGTM: Valuable addition of warnings for unused generated filesThis new section is a great addition to the script. It helps identify potential issues where generated Swagger files are not being included in the final output, which could lead to incomplete API documentation.
For consistency with the rest of the script, consider using the
VERBOSEflag here as well. This would allow users to control the verbosity of these warnings:if [ -n "$VERBOSE" ]; then for file in $swagger_files; do if ! grep -Fq "\"url\": \"$file\"" "$config_file"; then printf '\033[93mWARNING\033[0m: "%s" not referenced in %s\n' "$file" "$config_file" fi done fi
42-42: LGTM: Effective tag removal from Swagger fileThe grep command effectively removes the 'Query' and 'Service' tags from the combined Swagger file. The use of variables for input and output files is good practice.
For improved readability, consider breaking the long line into multiple lines:
grep -v \ -e ' - Query' \ -e ' - Service' \ "$temp_file" > "$output_file"This makes the patterns easier to read and modify if needed in the future.
1-45: Overall excellent improvements to the scriptThis refactoring of the
protoc-swagger-gen.shscript has resulted in significant improvements:
- Better variable management and naming conventions
- More robust proto file discovery and processing
- Addition of warnings for unused generated files
- Improved readability and maintainability throughout
These changes align well with the PR objectives, particularly in updating the Swagger generation process and improving the overall code quality. The script now provides a more reliable and maintainable solution for generating Swagger/OpenAPI documentation.
Consider adding a brief comment at the top of the script explaining its purpose and any dependencies (like the
swagger-combinetool). This would help future maintainers quickly understand the script's role in the project's build process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- scripts/protoc-swagger-gen.sh (1 hunks)
- scripts/protocgen.sh (1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/protocgen.sh (2)
Learnt from: SpicyLemon PR: provenance-io/provenance#2167 File: scripts/protocgen.sh:13-14 Timestamp: 2024-10-04T16:17:48.021Z Learning: In `scripts/protocgen.sh`, the regex pattern `'option go_package.*provenance'` is sufficient for selecting the necessary proto files and does not need modification.Learnt from: SpicyLemon PR: provenance-io/provenance#2167 File: scripts/protocgen.sh:15-17 Timestamp: 2024-10-04T16:14:41.003Z Learning: In the `scripts/protocgen.sh` script within the Provenance project, the paths involved will not have spaces, so handling spaces in filenames is not necessary.
🔇 Additional comments (6)
scripts/protocgen.sh (3)
9-11: LGTM: Proper implementation of verbose flag checkThe implementation of the verbose flag check is correct and follows POSIX-compliant shell scripting practices. Good job on using
=for string comparison instead of==.
13-14: LGTM: Efficient proto file selectionThe implementation for finding proto files is correct and efficient. The regex pattern
'option go_package.*provenance'is sufficient for selecting the necessary proto files in this project context.
15-17: LGTM: Effective proto file processingThe implementation for processing each proto file is correct and appropriate for this project's context. While the loop doesn't handle filenames with spaces, it's understood that this is not a concern in this codebase. The use of the
$VERBOSEflag for conditional output is a good practice.scripts/protoc-swagger-gen.sh (3)
3-5: LGTM: Improved verbose mode implementationThe introduction of the
VERBOSEflag with support for both-vand--verboseoptions is a good improvement. It follows bash conventions and provides better flexibility for users.
36-36: LGTM: Proper use of swagger-combine commandThe swagger-combine command is well-structured and uses the appropriate options. The use of variables makes the command more readable and maintainable.
44-45: LGTM: Proper cleanup with verbose outputThe cleanup command is well-implemented. It correctly uses the VERBOSE flag for output and uses the defined variable for the temporary directory. This ensures a clean workspace after script execution.
SpicyLemon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Makefile (1 hunks)
🧰 Additional context used
📓 Learnings (1)
Makefile (1)
Learnt from: SpicyLemon PR: provenance-io/provenance#2167 File: Makefile:494-497 Timestamp: 2024-10-04T16:14:19.504Z Learning: Swagger 2.0 is equivalent to OpenAPI v2, so files with 'swagger: "2.0"' are correctly using OpenAPI v2 specification.
🔇 Additional comments (5)
Makefile (5)
480-482: Improved Variable Naming and Docker Command SimplificationThe introduction of
protoVer,protoImageName, andprotoImagevariables enhances clarity and simplifies the Docker commands used for Protobuf generation. This refactoring promotes maintainability and readability across the Makefile.
490-490: Consistent Use of Docker Image for Protobuf Code GenerationUtilizing
$(protoImage)in theproto-gentarget ensures that Protobuf code generation is consistently executed within the specified Docker environment. This approach enhances reproducibility and minimizes environment-specific issues during the build process.
496-496: Consistent Use of Docker Image for Swagger GenerationApplying
$(protoImage)in theproto-swagger-gentarget standardizes the execution of Swagger generation within the Docker container. This consistency improves the reliability of the Swagger generation process.
513-513: Consistent Linting of Protobuf Files Using DockerThe use of
$(protoImage)to executebuf lintensures that Protobuf files are linted within a consistent Docker environment. This practice promotes uniformity and reduces discrepancies caused by local development environments.
504-509: Verify Availability of 'clang-format' in the Docker ImageThe
proto-formattarget invokesclang-formatwithin the Docker container specified by$(protoImage). Please ensure thatclang-formatis installed and accessible in the$(protoImageName)Docker image to prevent formatting errors during the Protobuf formatting step.Run the following script to verify that
clang-formatis available in the Docker image:
Description
closes: #2135
This PR does the following:
0.15.1(from0.14.0) and switches us to use openapiv2 instead of swagger.client/docs/swagger-ui/swagger.yamlfile and regenerates all the go code from our proto files.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores