Add remove_internal_comments option

[same-id]

To allow internal comments within the protobuf which won't make it the generated OAS file.

Using Google's AIP-192:

https://google.aip.dev/192#internal-comments

References to other Issues or PRs

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

Other comments

Summary by CodeRabbit

• New Feature: Introduced a new command-line flag remove_internal_comments which, when set, removes all substrings in comments that start with (-- and end with --). This feature aligns with AIP 192: Internal comments.
• New Feature: Added a new buf generate command to the Makefile for generating code based on a template and a protobuf file.
• Test: Added fuzz testing for the removeInternalComments function to ensure its correctness.
• Documentation: Updated OpenAPI output customization documentation to include the new remove_internal_comments option.

[coderabbitai]

Walkthrough

The changes introduce a new feature to remove internal comments from the OpenAPI output. This is achieved by adding new command-line options, modifying the Registry struct, and creating a new function to handle comment removal. The changes also include generated code for handling HTTP requests and forwarding them to gRPC endpoints.

Changes

File	Summary
Makefile, .../BUILD.bazel	Addition of a new buf generate command and inclusion of a new proto file in the proto_library.
.../remove_internal_comment.pb.gw.go	Generated code for handling HTTP requests and forwarding them to gRPC endpoints.
.../remove_internal_comment.proto	New protocol buffer file defining a gRPC service with specific comments to be excluded from the OpenAPI output.
internal/descriptor/registry.go	Introduction of two new fields and their getter and setter methods to the Registry struct.
protoc-gen-openapiv2/defs.bzl	New command-line options added to control the handling of comments in the generated OpenAPI output.
.../genopenapi/template.go	New function introduced to remove internal comments from a given string.
.../genopenapi/template_fuzz_test.go	New test function added to fuzz the removeInternalComments function.
protoc-gen-openapiv2/main.go	New command-line flag introduced to remove all substrings in comments that start with (-- and end with --).

🐇 "In the land of code so bright,

A rabbit hopped through day and night.

With each hop, a change was made,

In the world of software, a new layer laid.

Comments hidden, secrets kept,

Through lines of code, the rabbit leapt. 🐇"

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• Mention @coderabbitai in any review comment for bot assistance.
• Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

• Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

[coderabbitai]

Review Status

Actionable comments generated: 4

Commits

Files that changed from the base of the PR and between a39406f and 6b6d0a6 commits.

Files ignored due to filter (5)

• buf.yaml
• examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
• examples/internal/proto/examplepb/remove_internal_comment.pb.go
• examples/internal/proto/examplepb/remove_internal_comment.swagger.json
• examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go

Files selected for processing (8)

• Makefile (1 hunks)
• examples/internal/proto/examplepb/BUILD.bazel (1 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
• internal/descriptor/registry.go (2 hunks)
• protoc-gen-openapiv2/defs.bzl (4 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
• protoc-gen-openapiv2/main.go (2 hunks)

Files skipped from review due to trivial changes (4)

• examples/internal/proto/examplepb/BUILD.bazel
• examples/internal/proto/examplepb/remove_internal_comment.proto
• internal/descriptor/registry.go
• protoc-gen-openapiv2/internal/genopenapi/template.go

Additional comments (Suppressed): 7

Makefile (2)

• 118-123: The new buf generate commands for ignore_comment.proto and remove_internal_comment.proto are added correctly. However, ensure that the corresponding .proto files and their templates exist at the specified paths.

• 124-126: No issues found in these lines. The existing buf generate command for visibility_rule_echo_service.proto remains unchanged.

protoc-gen-openapiv2/main.go (1)

• 31-37: The new flag remove_internal_comments is introduced correctly and the description is clear. Ensure that this flag is documented in the user guide or any relevant documentation.

protoc-gen-openapiv2/defs.bzl (4)

• 60-67: The new command-line options ignore_comments and remove_internal_comments have been added correctly to the list of attributes. Ensure that these options are handled properly in the rest of the codebase.

• 112-123: The handling of the new command-line options ignore_comments and remove_internal_comments is implemented correctly here. The options are added to the args list if they are set.

• 234-241: The new command-line options ignore_comments and remove_internal_comments have been added correctly to the function call to _proto_gen_openapi_impl. Ensure that these options are handled properly in the function definition.

• 314-331: The new command-line options ignore_comments and remove_internal_comments have been defined correctly as boolean attributes with appropriate default values and documentation. The documentation for remove_internal_comments includes a link to the relevant Google AIP, which is helpful.

[coderabbitai]

Review Status

Actionable comments generated: 4

Commits

Files that changed from the base of the PR and between a39406f and 1bd8026 commits.

Files ignored due to filter (5)

• buf.yaml
• examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
• examples/internal/proto/examplepb/remove_internal_comment.pb.go
• examples/internal/proto/examplepb/remove_internal_comment.swagger.json
• examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go

Files selected for processing (8)

• Makefile (1 hunks)
• examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
• internal/descriptor/registry.go (2 hunks)
• protoc-gen-openapiv2/defs.bzl (4 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
• protoc-gen-openapiv2/main.go (2 hunks)

Files skipped from review due to trivial changes (4)

• Makefile
• examples/internal/proto/examplepb/BUILD.bazel
• internal/descriptor/registry.go
• protoc-gen-openapiv2/internal/genopenapi/template.go

Additional comments (Suppressed): 9

examples/internal/proto/examplepb/remove_internal_comment.proto (4)

• 1-39: Ensure that the new removeInternalComments function is correctly removing comments enclosed in (-- and --) from the generated OpenAPI documentation. Also, verify that the command-line flag for this feature is working as expected.

• 9-21: The service Foo2Service has been added. Ensure that it is correctly implemented and integrated with the rest of the system.

• 23-36: The message Foo2Request has been added. Make sure that it is being used correctly wherever required.

• 38-39: The message Foo2Reply has been added but it doesn't have any fields. If this is intentional, then it's fine. Otherwise, you might want to add some fields to it.

protoc-gen-openapiv2/defs.bzl (4)

• 60-67: The addition of ignore_comments and remove_internal_comments options is consistent with the PR summary. Ensure that these options are properly propagated to all functions that need them.

• 112-123: The code correctly adds the new command-line options to the protoc compiler arguments based on their values. However, ensure that the protoc-gen-openapiv2 plugin supports these options and behaves as expected when they are set.

• 234-241: The propagation of the new options to the _proto_gen_openapi_impl function is correct. Make sure that the function uses these options as intended.

• 314-331: The definitions of the new attributes for the Bazel rule are correct. The default values are appropriate (False), and the documentation is clear and helpful. However, ensure that the link provided in the doc for remove_internal_comments is accessible and contains relevant information.

protoc-gen-openapiv2/main.go (1)

• 131-136: The removeInternalComments option is set correctly in the registry. As discussed in the comment chain, ensure that the case where both ignoreComments and removeInternalComments are set to true is handled appropriately, with ignoreComments taking precedence as it ignores all comments, not just internal ones.

[johanbrandhorst]

First of all, sorry for the delay in reviewing this. I appreciate the detailed PR description. Apart from the suggestion to add a test, could you also add a new section to the "customize your openapi output" docs? Thanks!

[johanbrandhorst]

Also, thanks for being a test subject for the AI code review bot I installed 😁. Let me know what the experience is like as a contributor!

[coderabbitai]

Review Status

Actionable comments generated: 2

Commits

Files that changed from the base of the PR and between bcf125c and 3a2728f commits.

Files ignored due to filter (5)

• buf.yaml
• examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
• examples/internal/proto/examplepb/remove_internal_comment.pb.go
• examples/internal/proto/examplepb/remove_internal_comment.swagger.json
• examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go

Files selected for processing (9)

• Makefile (1 hunks)
• examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
• internal/descriptor/registry.go (2 hunks)
• protoc-gen-openapiv2/defs.bzl (4 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
• protoc-gen-openapiv2/main.go (2 hunks)

Files skipped from review due to trivial changes (5)

• Makefile
• examples/internal/proto/examplepb/BUILD.bazel
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go
• protoc-gen-openapiv2/defs.bzl
• protoc-gen-openapiv2/main.go

Additional comments (Suppressed): 7

examples/internal/proto/examplepb/remove_internal_comment.proto (4)

• 1-39: Ensure that the new protobuf file remove_internal_comment.proto is correctly imported and used in other parts of the codebase where necessary. Also, verify if the new service Foo2Service and its RPC method Foo2 are properly implemented and called.

• 9-21: The comments within the (-- --) brackets are intended to be excluded from the OpenAPI output as per the PR summary. Ensure that the implementation of this feature is working as expected by checking the generated OpenAPI documentation.

• 23-36: Similar to the previous comment, ensure that the internal comments in the message definitions Foo2Request and Foo2Reply are also being excluded from the OpenAPI output.

• 38-39: This comment should also be excluded from the OpenAPI output. Verify this in the generated documentation.

internal/descriptor/registry.go (2)

• 81-90: The new fields ignoreComments, removeInternalComments, and enumsAsInts are added to the Registry struct. Ensure that these fields are properly initialized in the constructor or wherever the Registry struct is instantiated.

• 588-596: Getter and setter methods for removeInternalComments are correctly implemented.

protoc-gen-openapiv2/internal/genopenapi/template.go (1)

• 2259-2262: Ensure that the GetRemoveInternalComments method is working as expected and that the flag is being set correctly in the command-line interface.

[coderabbitai]

Review Status

Actionable comments generated: 2

Commits

Files that changed from the base of the PR and between bcf125c and 7d99e1b commits.

Files ignored due to filter (5)

• buf.yaml
• examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
• examples/internal/proto/examplepb/remove_internal_comment.pb.go
• examples/internal/proto/examplepb/remove_internal_comment.swagger.json
• examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go

Files selected for processing (10)

• Makefile (1 hunks)
• examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
• internal/descriptor/registry.go (2 hunks)
• protoc-gen-openapiv2/defs.bzl (4 hunks)
• protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (1 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
• protoc-gen-openapiv2/main.go (2 hunks)

Files skipped from review due to trivial changes (6)

• Makefile
• examples/internal/proto/examplepb/BUILD.bazel
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go
• protoc-gen-openapiv2/defs.bzl
• protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel
• protoc-gen-openapiv2/main.go

Additional comments (Suppressed): 7

examples/internal/proto/examplepb/remove_internal_comment.proto (4)

• 1-39: Ensure that the new feature removeInternalComments is working as expected by verifying that all comments enclosed within (-- and --) are removed from the OpenAPI output when the flag is set.

• 9-21: The service Foo2Service and its RPC method Foo2 have been defined correctly. Ensure that the HTTP option for the RPC method matches the intended API route.

• 23-36: The message Foo2Request has been defined correctly with fields username and password. Make sure these field names align with the application's user model.

• 38-39: The message Foo2Reply is currently empty. If this is intentional and you're planning to use it as a placeholder for future data, then it's fine. Otherwise, consider adding necessary fields or remove it if it's not needed.

internal/descriptor/registry.go (2)

• 81-90: The addition of the removeInternalComments field in the Registry struct is a good approach to handle the removal of internal comments from the OpenAPI output. This allows for more granular control over comment handling, as opposed to using the existing ignoreComments field which would remove all comments.

• 588-596: The setter and getter methods for removeInternalComments are correctly implemented. They follow the same pattern as other similar methods in the Registry struct.

protoc-gen-openapiv2/internal/genopenapi/template.go (1)

• 2259-2262: Ensure that the GetRemoveInternalComments method is properly checking the flag and that the flag is being set correctly in the command-line arguments.

[johanbrandhorst]

Don't forget to add a section to the docs, too!

[Katarzynamm]

👍 czw., 7 wrz 2023, 22:58 użytkownik Harjot Gill ***@***.***> napisał:

…

***@***.**** commented on this pull request. ------------------------------ In protoc-gen-openapiv2/internal/genopenapi/template.go <#3560 (comment)> : > +func removeInternalComments(comment string) string { + c := []string{} + for len(comment) > 0 { + open := strings.SplitN(comment, "(--", 2) + ex := strings.TrimRight(open[0], " \t") + // Trim only one line prior to all spaces + switch { + case strings.HasSuffix(ex, "\r\n"): + ex = strings.TrimSuffix(ex, "\r\n") + case strings.HasSuffix(ex, "\n"): + ex = strings.TrimSuffix(ex, "\n") + } + if ex != "" { + c = append(c, ex) + } + if len(open) > 1 { + comment = open[1] + } else { + break + } + close := strings.SplitN(comment, "--)", 2) + if len(close) > 1 { + comment = close[1] + } else { + break + } + } + return strings.Join(c, "") +} Thanks for being patient test subjects! :) — Reply to this email directly, view it on GitHub <#3560 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUUANJVMGXZ2R2BOTEB5IUTXZIYPVANCNFSM6AAAAAA4FEMVWA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[coderabbitai]

Review Status

Actionable comments generated: 5

Commits

Files that changed from the base of the PR and between d476e2a and 4eeacf6 commits.

Files ignored due to filter (5)

• buf.yaml
• examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
• examples/internal/proto/examplepb/remove_internal_comment.pb.go
• examples/internal/proto/examplepb/remove_internal_comment.swagger.json
• examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go

Files selected for processing (11)

• Makefile (1 hunks)
• docs/docs/mapping/customizing_openapi_output.md (1 hunks)
• examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
• internal/descriptor/registry.go (2 hunks)
• protoc-gen-openapiv2/defs.bzl (4 hunks)
• protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (1 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
• protoc-gen-openapiv2/main.go (2 hunks)

Files skipped from review due to trivial changes (5)

• Makefile
• docs/docs/mapping/customizing_openapi_output.md
• examples/internal/proto/examplepb/BUILD.bazel
• protoc-gen-openapiv2/defs.bzl
• protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel

Additional comments (Suppressed): 13

examples/internal/proto/examplepb/remove_internal_comment.proto (4)

• 1-39: Ensure that the new remove_internal_comments flag is correctly implemented and tested to remove internal comments from the OpenAPI output. This includes verifying that all instances of (-- and --) are correctly identified and removed, and that no other parts of the comments are affected.

• 9-21: The service Foo2Service and its RPC method Foo2 have been added. Ensure that these changes are reflected in the rest of the codebase where necessary, including any routing or handler functions.

• 23-36: The message Foo2Request has been added with fields username and password. Make sure that these fields are handled correctly in the rest of the codebase, including any validation, serialization, and deserialization logic.

• 38-39: The message Foo2Reply has been added but it doesn't contain any fields. If this is intentional, ensure that the rest of the codebase handles empty responses correctly. If not, consider adding the necessary fields.

protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (2)

• 1-2: The build constraints are updated to Go 1.18. Ensure that the rest of the codebase and your deployment environments support this version.

• 13-26: This fuzz test is a good addition for testing the removeInternalComments function with various input scenarios. It compares the output of the function with the expected output generated by a regex, which is a good way to validate the correctness of the function.

internal/descriptor/registry.go (2)

• 81-90: The addition of the removeInternalComments field in the Registry struct is consistent with the existing structure and follows the same pattern as other fields like ignoreComments and enumsAsInts. This change aligns with the goal of the PR to add a feature for removing internal comments from the generated OpenAPI output.

• 585-600: The setter and getter methods for removeInternalComments are correctly implemented. They follow the same pattern as the existing methods for other fields in the Registry struct. The naming convention is consistent, and the methods do exactly what they're supposed to do: set and get the value of removeInternalComments.

protoc-gen-openapiv2/main.go (2)

• 31-37: The new flags removeInternalComments and enumsAsInts are introduced correctly. The descriptions are clear and provide enough context for their usage. However, as discussed in the comment chain, it would be beneficial to clarify the interaction between ignoreComments and removeInternalComments. If both flags are set to true, ignoreComments should take precedence because it ignores all comments, not just internal ones.

• 131-137: The removeInternalComments option is set correctly in the registry. As mentioned in the comment chain, ensure that the case where both ignoreComments and removeInternalComments are set to true is handled appropriately. In such a scenario, ignoreComments should take precedence as it ignores all comments, not just internal ones.

protoc-gen-openapiv2/internal/genopenapi/template.go (2)

• 2259-2263: Ensure that the GetRemoveInternalComments method is correctly implemented and tested. Also, verify that the removeInternalComments function is called only when necessary to avoid unnecessary computation.

• 2260-2263: The call to removeInternalComments function is correctly placed and will remove internal comments if the "remove_internal_comments" flag is set to true.

examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1)

• 1-171: The generated code looks good overall. It follows the standard conventions for gRPC and HTTP handlers in Go. The error handling is mostly correct, with a few minor issues noted above. The use of context cancellation and metadata propagation is correctly implemented.

[coderabbitai]

The error handling in this function could be improved. Currently, if an error occurs when decoding the request body, the function returns immediately with a status error. However, it would be better to close the reader before returning to ensure that resources are properly cleaned up.

	newReader, berr := utilities.IOReaderFactory(req.Body)
	if berr != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr)
	}
	if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF {
+		newReader.Close()
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
	}

[coderabbitai]

Similar to the previous comment, ensure to close the reader before returning in case of an error during decoding.

	newReader, berr := utilities.IOReaderFactory(req.Body)
	if berr != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr)
	}
	if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF {
+		newReader.Close()
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
	}

[coderabbitai]

This function dials a connection and then defers its closure. However, if dialing fails, there's no need to attempt to close the connection. The deferred closure should be moved inside the error check.

	conn, err := grpc.DialContext(ctx, endpoint, opts...)
	if err != nil {
		return err
	}
+	defer func() {
+		if cerr := conn.Close(); cerr != nil {
+			grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
+		}
+	}()
-	defer func() {
-		if err != nil {
-			if cerr := conn.Close(); cerr != nil {
-				grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
-			}
-			return
-		}
-		go func() {
-			<-ctx.Done()
-			if cerr := conn.Close(); cerr != nil {
-				grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
-			}
-		}()
-	}()

[same-id]

Don't forget to add a section to the docs, too!

Done

[johanbrandhorst]

Just one final nit, almost there :)

[coderabbitai]

Review Status

Actionable comments generated: 2

Commits

Files that changed from the base of the PR and between d476e2a and bb30237 commits.

Files ignored due to filter (6)

• buf.yaml
• examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
• examples/internal/proto/examplepb/remove_internal_comment.pb.go
• examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go
• examples/internal/proto/examplepb/remove_internal_comment.swagger.json
• examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go

Files selected for processing (10)

• Makefile (1 hunks)
• docs/docs/mapping/customizing_openapi_output.md (1 hunks)
• examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
• examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
• internal/descriptor/registry.go (2 hunks)
• protoc-gen-openapiv2/defs.bzl (4 hunks)
• protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (1 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
• protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
• protoc-gen-openapiv2/main.go (2 hunks)

Files skipped from review due to trivial changes (5)

• docs/docs/mapping/customizing_openapi_output.md
• examples/internal/proto/examplepb/BUILD.bazel
• protoc-gen-openapiv2/defs.bzl
• protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel
• protoc-gen-openapiv2/main.go

Additional comments (Suppressed): 10

Makefile (1)

• 118-123: Ensure that the new protobuf files ignore_comment.proto and remove_internal_comment.proto exist at the specified paths. Also, verify that the corresponding templates ignore_comment.buf.gen.yaml and remove_internal_comment.buf.gen.yaml are correctly set up to generate the desired output.

examples/internal/proto/examplepb/remove_internal_comment.proto (4)

• 1-39: The new proto file remove_internal_comment.proto is well structured and follows the protobuf syntax correctly. The comments that are supposed to be excluded from the OpenAPI output are correctly placed within (-- --) as per Google's AIP-192 for internal comments. Ensure that the new service Foo2Service and its RPC method Foo2 are properly integrated with the rest of the system.

• 9-21: Ensure that the new service Foo2Service and its RPC method Foo2 are correctly implemented in the server-side code. Also, verify that the HTTP POST route /v1/example/foo does not conflict with any existing routes.

• 23-36: The message definitions Foo2Request and Foo2Reply are correctly defined. However, ensure that these new messages are used correctly wherever required in the codebase.

• 38-39: The Foo2Reply message is currently empty. If this is intentional and you plan to use it as a placeholder for future fields, then it's fine. Otherwise, consider adding necessary fields to it.

internal/descriptor/registry.go (2)

• 81-90: The addition of the removeInternalComments field in the Registry struct is consistent with the PR's goal of removing internal comments from the generated OpenAPI output. The comment above the field provides a clear explanation of its purpose and references the relevant AIP for further context.

• 585-600: The setter and getter methods for removeInternalComments are correctly implemented. They follow the same pattern as the existing methods for other fields in the Registry struct.

protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (2)

• 1-2: The build constraints are updated to Go 1.18. Ensure that the rest of the codebase and your deployment environment support this version.

• 13-26: This fuzz test function FuzzRemoveInternalComments is added to test the removeInternalComments function. It adds several cases to the fuzzer, then checks that the output of removeInternalComments(s) is equal to the result of removing all matches of replaceInternalCommentsRegex from s. If they're not equal, it reports an error. This seems like a good way to ensure that removeInternalComments behaves as expected.

protoc-gen-openapiv2/internal/genopenapi/template.go (1)

• 2257-2263: The removeInternalComments function is correctly used in updateOpenAPIDataFromComments. It is only called if the "remove_internal_comments" flag is set to true, which is the expected behavior.

[coderabbitai]

This regular expression is used to match internal comments. It seems to correctly identify substrings that start with (-- and end with --), but it also matches any line that starts with (-- and does not have a closing --). This could lead to unexpected behavior if there's a typo in the comment syntax or if the closing tag is missing. Consider adjusting the regex to only match properly closed comments.

[johanbrandhorst]

I am not a regex expert, but is this right? It looks to me like it might be since the 3rd capturing group allows --) or .*?

[coderabbitai]

@johanbrandhorst: You're correct in your observation. The regular expression is designed to match two scenarios:

1. A substring that starts with (--, ends with --), and may contain any characters (including newlines) in between due to the .*? part. This is the expected format for internal comments.

2. A line that starts with (-- but does not have a closing --). In this case, it matches until the end of the string (.*$). This is to handle cases where there might be a typo or missing closing tag.

The | operator in the regex means "or", so it will match either of these two scenarios. However, because of the ? after the second group, the regex engine will try to match the first scenario before falling back to the second one if the first doesn't match. This means it will correctly identify properly closed comments before considering unclosed ones.

So yes, the regular expression is working as intended based on the requirements. However, if you want to strictly enforce the comment syntax and avoid matching lines with missing closing tags, you could remove the |.*$ part from the regex. This would make it only match substrings that start with (-- and end with --).

[johanbrandhorst]

See my question on the regex

[same-id]

See my question on the regex

it's or .*$

Meaning, prefer a closing bracket - but if it wasn't closed, just remove until the end.

Oh, how much have I fought with this with the no-lookahead RE2 syntax

[same-id]

Also you can thank the fuzzer test to make sure that the regex is absolutely right :D

[johanbrandhorst]

Thanks for your contribution!