Add AccountCreationSchema to ConnectorMetadata #285
Conversation
|
Warning Rate limit exceeded@danlarkin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe changes introduce a comprehensive account creation schema for connector metadata, defining new message types that support various field types such as strings, booleans, lists, integers, and maps. The implementation adds robust validation mechanisms to ensure data integrity during account creation. New methods for validation are added to the Changes
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 (
|
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pb/c1/connector/v2/connector.pb.validate.go (1)
1816-2976: Consider reducing code duplication in validation error types.The validation functions and error types for
AccountCreationSchemaand related messages (AccountCreationField,AccountCreationStringField, etc.) are repetitive. Refactoring these to reduce duplication can enhance maintainability.You might consider creating generic validation error types or helper functions that can handle common validation patterns across different message types. This adheres to the DRY (Don't Repeat Yourself) principle and makes future maintenance easier.
proto/c1/connector/v2/connector.proto (2)
130-135: Specify validation rules forAccountCreationStringField.Currently, the
value_validatorfield inAccountCreationStringFieldreferencesvalidate.StringRules. Ensure that appropriate string validation rules are set to enforce constraints like minimum and maximum length, regex patterns, etc.Defining explicit validation rules enhances data integrity and helps prevent invalid data from being processed.
137-140: Remove unusedvaluefield inAccountCreationBoolField.The
AccountCreationBoolFieldmessage contains avaluefield that may not be necessary in this context since the actual value would likely be provided at runtime.Consider removing the
valuefield or clarifying its purpose to avoid confusion.message AccountCreationBoolField { - bool value = 1; optional bool default_value = 2; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
pb/c1/connector/v2/connector.pb.validate.go(2 hunks)proto/c1/connector/v2/connector.proto(2 hunks)
🔇 Additional comments (5)
pb/c1/connector/v2/connector.pb.validate.go (1)
557-584: Validation added forAccountCreationSchemainConnectorMetadata.The added code correctly implements validation for the new
AccountCreationSchemafield in theConnectorMetadatamessage. This ensures that any embedded messages are properly validated, maintaining data integrity.proto/c1/connector/v2/connector.proto (4)
48-48: Addition ofaccount_creation_schematoConnectorMetadata.The inclusion of the
AccountCreationSchemafield in theConnectorMetadatamessage enhances the schema to support detailed account creation configurations.Ensure that this addition maintains backward compatibility. Since the new field is presumably optional, existing consumers of
ConnectorMetadatashould not be impacted. However, it's prudent to verify that all systems interacting with this message can handle the new field gracefully.
110-155: Well-structured definition of account creation messages.The new messages
AccountCreationSchema,AccountCreationField, and their related types are well-defined. They provide flexibility for various field types and validation rules, allowing for comprehensive account creation configurations.
115-128: Clarify the use ofoneofinAccountCreationField.The
AccountCreationFieldmessage uses aoneofto represent different field types. Ensure that the handling of thisoneofis correctly implemented in the codebase, and consider providing documentation or comments for clarity.It's important to confirm that serialization and deserialization correctly manage the
oneoffields. Additionally, make sure that any code handlingAccountCreationFieldaccounts for the possibility of different field types.
152-155: Ensure proper validation forAccountCreationMapField.The
AccountCreationMapFieldincludes avalue_validatorof typevalidate.MapRules. Double-check that the map validation rules are properly defined to enforce key and value constraints.Define specific rules for map keys and values to prevent invalid data entries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proto/c1/connector/v2/connector.proto (1)
123-129: Consider renaming fields in theoneofdeclaration for clarity and to avoid potential keyword conflicts.Using field names like
bool,i, andmwithin theoneof fieldmay lead to confusion or conflicts with reserved keywords in some languages or tools. Renaming these fields to more descriptive names such asbool_field,int_field, andmap_fieldcan enhance readability and maintainability.Apply this diff to rename the fields:
oneof field { StringField str = 100; - BoolField bool = 101; + BoolField bool_field = 101; StringListField str_list = 102; - IntField i = 103; + IntField int_field = 103; - MapField m = 104; + MapField map_field = 104; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
pb/c1/connector/v2/connector.pb.validate.go(2 hunks)proto/c1/connector/v2/connector.proto(3 hunks)proto/c1/connector/v2/resource.proto(1 hunks)proto/c1/connector/v2/ticket.proto(0 hunks)proto/c1/connectorapi/baton/v1/baton.proto(6 hunks)
💤 Files with no reviewable changes (1)
- proto/c1/connector/v2/ticket.proto
✅ Files skipped from review due to trivial changes (2)
- proto/c1/connector/v2/resource.proto
- proto/c1/connectorapi/baton/v1/baton.proto
🔇 Additional comments (5)
pb/c1/connector/v2/connector.pb.validate.go (4)
557-584: Validation logic forAccountCreationSchemais correctly integrated.The added validation code for
AccountCreationSchemain theConnectorMetadatastruct follows the existing validation patterns. It appropriately checks for bothValidateAll()andValidate()methods and handles validation errors consistently with other fields.
1817-1955:Validatemethods forConnectorAccountCreationSchemaare properly implemented.The
ValidateandValidateAllmethods forConnectorAccountCreationSchemacorrectly implement the validation logic. The code iterates over theFieldMap, sorts the keys for consistent order, and validates eachFieldentry, handling errors as expected.
1966-2292: Ensure comprehensive validation inConnectorAccountCreationSchema_Field.The validation function for
ConnectorAccountCreationSchema_Fieldcorrectly handles theoneoffield by checking for typed-nil values and validating the embedded field types. This ensures that only one field is set and that it is valid, maintaining the integrity of the schema.
2293-2733: Nested field types have appropriate validation methods.The
Validatemethods for the nested field types (StringField,StringListField,BoolField,IntField,MapField) are correctly defined. They handle default values appropriately and ensure that any embedded data is validated according to the defined rules.proto/c1/connector/v2/connector.proto (1)
132-147: Verify the use of theoptionalkeyword for field declarations in proto3.The
optionalkeyword was reintroduced in proto3 with version 3.12 of the Protocol Buffers compiler (protoc). Ensure that your development environment and tooling support protoc version 3.12 or higher to prevent any compatibility issues.Run the following script to check the installed
protocversion:✅ Verification successful
The use of
optionalkeyword in proto3 is correctly implemented.The codebase demonstrates proper usage of proto3 syntax with optional fields. The explicit
syntax = "proto3";declaration and consistent field definitions across multiple proto files indicate a well-structured protobuf implementation. The presence of protobuf-related dependencies in the vendor directory suggests proper dependency management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the installed version of protoc compiler. # Test: Get the protoc version. Expected: Version 3.12 or higher. protoc --versionLength of output: 115
Script:
#!/bin/bash # Check proto syntax version and search for build configs echo "=== Checking proto syntax ===" rg "syntax = \"proto3\"" proto/c1/connector/v2/connector.proto -A 1 echo -e "\n=== Searching for build configurations ===" fd "BUILD|.bazel|CMakeLists.txt|Makefile|package.json" --type f echo -e "\n=== Looking for similar optional usage ===" rg "optional \w+" --type protoLength of output: 1263
Script:
#!/bin/bash # Find and examine proto files and build configurations echo "=== Finding all proto files ===" fd '\.proto$' echo -e "\n=== Checking proto files content ===" fd '\.proto$' -x cat {} echo -e "\n=== Checking build files for protobuf version ===" rg -i "protobuf|protoc" MakefileLength of output: 79961
| } | ||
|
|
||
| message StringListField { | ||
| // validator rules for value. may be empty. |
There was a problem hiding this comment.
This comment seems out of date
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proto/c1/connector/v2/connector.proto (2)
9-9: Unused import:google/protobuf/timestamp.proto.The imported
timestamp.protodoes not appear to be used in this file. Consider removing it to clean up unnecessary dependencies.
121-121: Verify the necessity of thedeprecatedfield in theFieldmessage.The
Fieldmessage includes a booleandeprecatedfield. If its purpose is to indicate deprecation status, consider using the protobuf built-in option[deprecated = true]on fields to leverage compiler warnings and tooling support. If it serves a different purpose, ensure that this is clearly documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
pb/c1/connector/v2/connector.pb.validate.go(2 hunks)proto/c1/connector/v2/connector.proto(3 hunks)
🔇 Additional comments (7)
pb/c1/connector/v2/connector.pb.validate.go (3)
557-584: Validation logic forAccountCreationSchemais correctly implemented.The validation methods for
AccountCreationSchemawithinConnectorMetadataare consistent with existing patterns and ensure proper error handling.
1816-2876: Validation methods forConnectorAccountCreationSchemaand nested fields are well-implemented.The added validation functions provide comprehensive checks for the new message structures, maintaining consistency and completeness in validation.
1840-1846: Good practice: Sorting map keys for deterministic iteration.Sorting the keys in
FieldMapensures consistent validation order, which is beneficial for testing and debugging.proto/c1/connector/v2/connector.proto (4)
49-49: Addition ofaccount_creation_schemafield is appropriate.The new field
account_creation_schemais correctly added toConnectorMetadatawith field number 9, following proper sequencing.
111-150: Definition ofConnectorAccountCreationSchemaand nested messages is well-structured.The message definitions are clear and align with protobuf best practices, enhancing the schema for account creation configurations.
131-145: Verify the usage of theoptionalkeyword in proto3 messages.The
optionalkeyword is used in thedefault_valuefields ofStringField,BoolField, andIntField. Ensure that your protobuf compiler and code generation tools supportoptionalfields in proto3 syntax to avoid compatibility issues.
147-149: Consider potential recursive definitions inMapField.The
MapFieldincludes adefault_valueof typemap<string, Field>, which refers back toField. Ensure that this recursive structure is intended and that serialization and deserialization handle it correctly to prevent runtime errors.
danlarkin
force-pushed
the
danlarkin/duct-9356-account-creation-schema
branch
from
9ea719c to
13da073
Compare
| @@ -6,6 +6,7 @@ import "c1/connector/v2/asset.proto"; | |||
| import "c1/connector/v2/resource.proto"; | |||
| import "google/protobuf/any.proto"; | |||
| import "google/protobuf/struct.proto"; | |||
| import "google/protobuf/timestamp.proto"; | |||
There was a problem hiding this comment.
| import "google/protobuf/timestamp.proto"; |
There was a problem hiding this comment.
Guess this breaks lint sadge
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
pb/c1/connector/v2/connector.pb.validate.go(2 hunks)proto/c1/connector/v2/annotation_trait.proto(1 hunks)proto/c1/connector/v2/connector.proto(3 hunks)proto/c1/connector/v2/resource.proto(1 hunks)proto/c1/connector/v2/ticket.proto(0 hunks)proto/c1/connectorapi/baton/v1/baton.proto(6 hunks)
💤 Files with no reviewable changes (1)
- proto/c1/connector/v2/ticket.proto
✅ Files skipped from review due to trivial changes (1)
- proto/c1/connector/v2/annotation_trait.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- proto/c1/connector/v2/resource.proto
- proto/c1/connectorapi/baton/v1/baton.proto
🧰 Additional context used
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/connector/v2/connector.proto
[failure] 9-9:
Import "google/protobuf/timestamp.proto" is unused.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (4)
pb/c1/connector/v2/connector.pb.validate.go (2)
557-584: Validation logic forAccountCreationSchemais correctly implemented.The validation logic for the new
AccountCreationSchemafield inConnectorMetadatacorrectly follows the established pattern, ensuring proper validation of embedded messages.
1816-2882: Validation methods forConnectorAccountCreationSchemaand associated fields are properly added.The validation methods (
Validate,ValidateAll, etc.) forConnectorAccountCreationSchemaand its nested messages (Field,StringField,BoolField,StringListField,IntField,MapField) are correctly implemented, ensuring comprehensive validation and error handling.proto/c1/connector/v2/connector.proto (2)
49-49: Addition ofaccount_creation_schemafield toConnectorMetadatais appropriate.The new field
account_creation_schemais correctly added toConnectorMetadatawith field number 9, extending the metadata structure to support account creation configurations.
111-151: Definition ofConnectorAccountCreationSchemaand its nested messages is correct.The messages
ConnectorAccountCreationSchema,Field,StringField,BoolField,StringListField,IntField, andMapFieldare properly defined, following protobuf best practices. The use of field numbers, including starting theoneoffields at 100, is appropriate and helps maintain clarity in the message definitions.
| @@ -6,6 +6,7 @@ | |||
| import "c1/connector/v2/resource.proto"; | |||
| import "google/protobuf/any.proto"; | |||
| import "google/protobuf/struct.proto"; | |||
| import "google/protobuf/timestamp.proto"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import google/protobuf/timestamp.proto.
The import of google/protobuf/timestamp.proto is unused in this file and can be removed to clean up the code.
Do you want me to generate a diff to remove the unused import?
🧰 Tools
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 9-9:
Import "google/protobuf/timestamp.proto" is unused.
Extend
ConnectorMetadatamessage with a new field of typeAccountCreationSchemato capture arbitrary account creation details.Summary by CodeRabbit
New Features
Improvements