Custom flag validation #303
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant modifications across several files, primarily focusing on restructuring command-line options and enhancing server configuration validation. The Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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
🧹 Outside diff range and nitpick comments (5)
pkg/config/validation.go (1)
14-33: Refactor: Consolidate Repetitive Validation ChecksThe validation for
DB.WriterConnectionStringis repeated forReplication,Sync, andIndexer. Consider consolidating these checks to reduce code duplication.Apply this refactor to simplify the validation:
if options.Replication.Enable || options.Sync.Enable || options.Indexer.Enable { if options.DB.WriterConnectionString == "" { return errors.New("DB.WriterConnectionString is required") } }cmd/cli/options.go (2)
14-17: Consider Marking Mandatory Fields as RequiredIf
AdminPrivateKeyandNodeIdare mandatory forUpdateHealthOptions, consider addingrequired:"true"to enforce this at the command-line parsing level.
19-23: Consider Marking Mandatory Fields as RequiredIn
UpdateAddressOptions, addingrequired:"true"toPrivateKey,NodeId, andAddresswill ensure these fields are provided when the command is used.cmd/cli/main.go (2)
Line range hint
46-85: Consider improving error message consistency and function structure.
Error message capitalization is inconsistent:
- Some use "Could not..."
- Others use "could not..."
The function handles multiple responsibilities and could benefit from decomposition.
Consider:
- Standardizing error message format:
- return nil, fmt.Errorf("Could not parse options: %s", err) + return nil, fmt.Errorf("could not parse options: %s", err)
- Breaking down the function into smaller, focused functions:
func addCommands(parser *flags.Parser, options *CLI) error { // Add all commands } func validateParserResults(parser *flags.Parser) error { // Handle parsing results } func parseOptions(args []string) (*CLI, error) { options := initializeOptions() parser := flags.NewParser(&options.GlobalOptions, flags.Default) if err := addCommands(parser, options); err != nil { return nil, err } if err := validateParserResults(parser, args); err != nil { return nil, err } return options, nil }
Line range hint
1-248: Overall structure looks good with room for future improvements.The refactoring of the CLI options structure is well-executed. Future improvements could include:
- Breaking down the large command handler functions (e.g.,
registerNode,updateHealth) into smaller, more focused functions- Adding validation for the HTTP addresses in
registerNodeandupdateAddress- Consider introducing interfaces for better testing and dependency injection
Consider introducing a
Registryinterface to abstract the blockchain operations:type Registry interface { AddNode(ctx context.Context, owner string, key *ecdsa.PublicKey, addr string) error UpdateHealth(ctx context.Context, nodeID string, health bool) error // ... other methods }This would:
- Make the code more testable
- Allow for different implementations (e.g., for testing or different backends)
- Simplify the command handlers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
cmd/cli/main.go(2 hunks)cmd/cli/options.go(1 hunks)cmd/replication/main.go(4 hunks)pkg/config/options.go(3 hunks)pkg/config/validation.go(1 hunks)pkg/server/server.go(4 hunks)
🔇 Additional comments (14)
pkg/server/server.go (4)
85-95: Conditional Initialization of 'Registrant' Service
The Registrant service is now initialized only when Replication or Sync is enabled, which optimizes resource usage by avoiding unnecessary initialization.
99-107: Initialization of validationService for Indexer
The validationService is correctly initialized using mlsvalidate.NewMlsValidationService, ensuring messages are validated properly when the Indexer is enabled.
122-137: Refactor: Simplify API Server Start Logic
Extracting the API server startup into the startAPIServer function improves code readability and separation of concerns.
138-152: Conditional Initialization of SyncServer
The SyncServer is initialized only when Sync is enabled, which is appropriate and conserves resources.
pkg/config/validation.go (1)
8-12: Consistent Validation of Payer Private Key
The check for options.Payer.PrivateKey when Payer is enabled is appropriate to prevent misconfiguration.
cmd/cli/options.go (2)
25-27: Approved: Required Annotation for Private Key
The GetPubKeyOptions struct correctly marks PrivateKey as required, ensuring the necessary information is supplied.
29-34: Approved: Required Fields in RegisterNodeOptions
All necessary fields in RegisterNodeOptions are appropriately marked as required, which will enforce proper usage of the command.
cmd/replication/main.go (3)
40-44: Validation of Server Options
Calling config.ValidateServerOptions(options) ensures that the server will not start with invalid configurations, preventing runtime errors due to misconfiguration.
66-77: Conditional Database Initialization
The database is now initialized only if Replication, Sync, or Indexer are enabled, optimizing resource usage.
68-77: Error Handling with Database Initialization
Proper error handling is in place for database initialization failures, logging the error and terminating the application if necessary.
pkg/config/options.go (2)
23-26: Removal of required:"true" Tags from DbOptions
The required:"true" tags have been removed from DbOptions fields. Since validation is now explicitly handled in ValidateServerOptions, this change centralizes validation logic.
42-42: Removal of required:"true" Tag from PayerOptions.PrivateKey
Removing the required:"true" tag is acceptable because the new validation function ensures that PrivateKey is provided when the Payer is enabled.
cmd/cli/main.go (2)
38-44: LGTM: Option initialization looks correct.
The variable declarations properly align with the CLI struct fields and use the new local option types.
18-25: Verify the new option types and potential name conflicts.
The embedding of GlobalOptions and removal of the config. prefix suggests these types have been moved locally. Please ensure:
- All option types (
GetPubKeyOptions,GenerateKeyOptions, etc.) are properly defined in the new location - No field name conflicts exist between the embedded
GlobalOptionsand theCLIstruct
✅ Verification successful
All option types are properly defined and no conflicts found
The verification shows:
- All option types are properly defined in
cmd/cli/options.go GlobalOptionsonly containsContractsandLogfields which don't conflict with any fields in theCLIstruct- The removal of the
config.prefix is valid as these types are now locally defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the option types are defined in the new location
fd -e go -x grep -l "type.*Options struct" {}
# Check for potential field name conflicts
ast-grep --pattern 'type GlobalOptions struct {
$$$
}'
Length of output: 456
Script:
#!/bin/bash
# Check the definition of all option types in cmd/cli/options.go
cat cmd/cli/options.go
# Check for any field name conflicts between CLI struct and GlobalOptions
ast-grep --pattern 'type CLI struct {
$$$
}'
Length of output: 2110
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
pkg/config/validation.go (4)
8-11: Add function documentation.Please add a documentation comment that describes the function's purpose, parameters, and return values. This will help other developers understand how to use this validation function.
+// ValidateServerOptions validates the provided ServerOptions configuration. +// It checks for required fields and valid values across different option sections. +// Returns an error if any validation fails, with a detailed message of all validation errors. func ValidateServerOptions(options ServerOptions) error {
40-44: Standardize flag naming in error messages.For consistency with other error messages, use kebab-case format for flag names.
if options.Payer.Enable { if options.Payer.PrivateKey == "" { - missingSet["--payer.PrivateKey"] = struct{}{} + missingSet["--payer.private-key"] = struct{}{} } }
67-87: Consider using structured validation errors.The current error handling concatenates strings, which makes it harder for callers to programmatically handle specific validation failures. Consider using a custom validation error type.
+type ValidationError struct { + MissingFields []string + CustomErrors []string +} + +func (v ValidationError) Error() string { + var parts []string + if len(v.MissingFields) > 0 { + parts = append(parts, "Missing required arguments: " + strings.Join(v.MissingFields, ", ")) + } + if len(v.CustomErrors) > 0 { + parts = append(parts, strings.Join(v.CustomErrors, "; ")) + } + return strings.Join(parts, "; ") +} if len(missingSet) > 0 || len(customSet) > 0 { - var errs []string + ve := ValidationError{} if len(missingSet) > 0 { - var errorMessages []string - errorMessages = append(errorMessages, "Missing required arguments:") for err := range missingSet { - errorMessages = append(errorMessages, err) + ve.MissingFields = append(ve.MissingFields, err) } - errs = append(errs, strings.Join(errorMessages, ", ")) } if len(customSet) > 0 { for err := range customSet { - errs = append(errs, err) + ve.CustomErrors = append(ve.CustomErrors, err) } } - return errors.New(strings.Join(errs, "; ")) + return ve }
8-87: Consider architectural improvements and add unit tests.
- The validation function handles multiple concerns. Consider splitting it into smaller, focused validators for each section (Contracts, Payer, etc.).
- Add unit tests to verify validation behavior for various scenarios:
- All valid options
- Missing required fields
- Invalid values
- Different combinations of enabled features
Would you like me to help with:
- Restructuring the validation into smaller, focused validators?
- Creating a comprehensive test suite?
mkysel
force-pushed
the
mkysel/custom-flag-validation
branch
from
4c5ce4a to
b90e1e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
cmd/cli/main.go (1)
38-44: Consider enhancing the command registration logic.While the option type changes look good, there are opportunities to improve the command registration:
- The repeated command registration pattern could be refactored to reduce duplication
- Error messages could be more descriptive by including the specific validation failure
- Consider adding explicit validation for required fields
Here's a suggested approach to reduce duplication:
type commandDef struct { name string description string options interface{} } func (cli *CLI) registerCommands(parser *flags.Parser) error { commands := []commandDef{ {"generate-key", "Generate a public/private keypair", &cli.GenerateKey}, {"register-node", "Register a node", &cli.RegisterNode}, // ... other commands } for _, cmd := range commands { if _, err := parser.AddCommand(cmd.name, cmd.description, "", cmd.options); err != nil { return fmt.Errorf("failed to add command %s: %w", cmd.name, err) } } return nil }pkg/server/server.go (2)
99-106: Consider validating MlsValidation options before use.While the validation service initialization looks correct, it might be beneficial to validate the
MlsValidationoptions before using them to create the service.Consider adding validation in
ValidateServerOptions:func ValidateServerOptions(options ServerOptions) error { // existing validation... + if options.Indexer.Enable { + if err := validateMlsOptions(options.MlsValidation); err != nil { + return fmt.Errorf("invalid MLS validation options: %w", err) + } + } return nil }
Line range hint
156-217: Consider refactoring service registration for better maintainability.While the function is well-structured, consider these improvements:
- Split the service registration closure into separate functions for each service
- Avoid redeclaring the
errvariable in the closureHere's a suggested refactoring:
func startAPIServer(...) error { var err error - serviceRegistrationFunc := func(grpcServer *grpc.Server) error { - if options.Replication.Enable { - replicationService, err := message.NewReplicationApiService(...) - if err != nil { - return err - } - message_api.RegisterReplicationApiServer(grpcServer, replicationService) - log.Info("Replication service enabled") - } - if options.Payer.Enable { - // ... payer service initialization - } - return nil - } + serviceRegistrationFunc := createServiceRegistrationFunc(ctx, log, options, s, writerDB, blockchainPublisher) s.apiServer, err = api.NewAPIServer(...) return err } +func createServiceRegistrationFunc(...) func(*grpc.Server) error { + return func(grpcServer *grpc.Server) error { + if err := registerReplicationService(...); err != nil { + return err + } + if err := registerPayerService(...); err != nil { + return err + } + return nil + } +} + +func registerReplicationService(...) error { + if !options.Replication.Enable { + return nil + } + // Replication service initialization +} + +func registerPayerService(...) error { + if !options.Payer.Enable { + return nil + } + // Payer service initialization +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
cmd/cli/main.go(2 hunks)cmd/cli/options.go(1 hunks)cmd/replication/main.go(4 hunks)pkg/config/options.go(3 hunks)pkg/config/validation.go(1 hunks)pkg/server/server.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/replication/main.go
- pkg/config/validation.go
- cmd/cli/options.go
- pkg/config/options.go
🔇 Additional comments (5)
cmd/cli/main.go (1)
18-25: LGTM! Good structural improvements to the CLI type.
The changes improve code organization by:
- Embedding GlobalOptions directly
- Moving option types locally for better encapsulation
Let's verify the option types are properly defined:
✅ Verification successful
CLI refactoring verified successfully
The verification confirms:
- All command-specific option types are properly defined in
cmd/cli/options.go - The remaining
configpackage references are valid and intentional, specifically forContractsOptionsandLogOptionswhich are well-defined inpkg/config/options.go - No other unexpected
configpackage references exist in the CLI code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify option types are properly defined in the new file
# and there are no lingering references to the old config package
# Check for option type definitions
rg "type.*Options struct" cmd/cli/
# Verify no lingering config package references
rg "config\.[A-Za-z]+Options" cmd/cli/
Length of output: 640
Script:
#!/bin/bash
# Let's verify if the config package references are valid imports
# and check the structure of these options
# Check imports in options.go
rg "^import.*config" cmd/cli/options.go -A 5
# Check the structure of ContractsOptions and LogOptions in config package
fd "config" --type f --exec rg "type (Contracts|Log)Options struct" {} \;
Length of output: 360
Script:
#!/bin/bash
# Let's try a different approach to find these option types
# by searching in the pkg/config directory specifically
# Check for the options types in pkg/config
rg "type (Contracts|Log)Options struct" pkg/config/ -A 3
# Also verify there are no other config package references
# in the CLI code that might need attention
rg "config\." cmd/cli/ -l
Length of output: 1302
pkg/server/server.go (4)
6-6: LGTM: Import added for new validation service.
122-136: LGTM: Clean API server initialization.
The conditional initialization and error handling look good. The logging provides clear feedback about the server state.
138-151: LGTM: Clean sync server initialization.
The conditional initialization, dependency injection, and error handling are well implemented.
85-95: Verify feature flag combinations.
The registrant initialization is now conditional on either Replication or Sync being enabled, which is logically correct. However, let's verify all possible feature flag combinations are handled properly.
✅ Verification successful
Let me analyze the usage of these feature flags to verify their combinations.
Let me try a different approach to check the configuration structure.
Based on the results from the first search, I can now generate a final response as we have enough information about how these feature flags are used together.
Feature flag combinations are properly handled
The codebase shows consistent and correct handling of the Replication and Sync feature flags:
- Registrant is correctly initialized when either flag is enabled
- Each feature has proper validation in config/validation.go ensuring required dependencies
- The features are independently handled in their respective initialization blocks
- Database connections are properly set up when either feature is enabled
- All combinations maintain proper dependency chain (registrant -> sync/replication services)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all places where these feature flags are used together
rg -A 5 "options\.(Replication|Sync)\.Enable"
Length of output: 2168
Script:
#!/bin/bash
# Let's check the configuration structure and defaults
ast-grep --pattern 'type Options struct {
$$$
}'
Length of output: 52
Script:
#!/bin/bash
# Search for the Options struct definition using ripgrep
rg -A 20 "type Options struct"
Length of output: 32
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
pkg/config/cliOptions.go (2)
8-10: Add documentation for empty option structsThe purpose of these empty structs is not immediately clear. Consider adding documentation to explain their intended use and why they don't require any fields.
+// GenerateKeyOptions defines options for key generation operations. type GenerateKeyOptions struct{} +// GetAllNodesOptions defines options for retrieving all nodes. type GetAllNodesOptions struct{}
1-32: Consider adding constants for validation patternsTo improve maintainability and ensure consistency across the codebase, consider extracting the validation patterns into constants.
package config +const ( + // Validation patterns for CLI options + PrivateKeyPattern = "^0x[a-fA-F0-9]{64}$" + PublicKeyPattern = "^0x[a-fA-F0-9]{128}$" + AddressPattern = "^https?://[\\w.-]+(:\\d+)?$" + BlockchainAddrPattern = "^0x[a-fA-F0-9]{40}$" +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/cli/main.go(2 hunks)pkg/config/cliOptions.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cli/main.go
🔇 Additional comments (1)
pkg/config/cliOptions.go (1)
3-6: Verify the existence of referenced option types
The structure looks well-organized with proper namespace separation. However, let's verify the referenced types exist.
✅ Verification successful
Referenced types are properly defined
Both ContractsOptions and LogOptions are well-defined in pkg/config/options.go with appropriate fields and configuration tags.
ContractsOptionsis defined with blockchain-related configuration optionsLogOptionsis defined with logging-related configuration options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ContractsOptions and LogOptions are defined
# Expected: Find definitions of these types in the codebase
echo "Checking for ContractsOptions..."
ast-grep --pattern 'type ContractsOptions struct { $$$ }'
echo "Checking for LogOptions..."
ast-grep --pattern 'type LogOptions struct { $$$ }'
Length of output: 2430
| var dbInstance *sql.DB | ||
| if options.Replication.Enable || options.Sync.Enable || options.Indexer.Enable { |
There was a problem hiding this comment.
I'm wondering if it makes sense to use a singleton pattern for the DB, and just instantiate it wherever it is needed, rather than trying to comprehensively enumerate all the cases here.
Or: Just instantiate the DB if the DB params are specified, and don't instantiate if it's not, and throw an error if a DB is used somewhere when it hasn't been instantiated.
There was a problem hiding this comment.
I will give it some thought tomorrow!
Summary by CodeRabbit
New Features
Bug Fixes
Chores