feat/bug: Add JSON schema + add missing pool default init #121
Conversation
fix missing pool default init
nickdnk
added
bug
WalkthroughThe changes introduced in this pull request involve modifications to the Changes
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: 6
🧹 Outside diff range and nitpick comments (3)
config.go (3)
Line range hint
56-75: Simplify TLS validation logic and remove unnecessary linter directive.The TLS validation block can be simplified, and the
//nolint:nestifcomment is no longer necessary as the nesting isn't complex.- if c.TLS != nil { //nolint:nestif - if _, err := os.Stat(c.TLS.Key); err != nil { - if os.IsNotExist(err) { - return errors.E(op, errors.Errorf("key file '%s' does not exists", c.TLS.Key)) - } - - return errors.E(op, err) - } - - if _, err := os.Stat(c.TLS.Cert); err != nil { - if os.IsNotExist(err) { - return errors.E(op, errors.Errorf("cert file '%s' does not exists", c.TLS.Cert)) - } - - return errors.E(op, err) - } - } + if c.TLS == nil { + return nil + } + + for file, fileType := range map[string]string{ + c.TLS.Key: "key", + c.TLS.Cert: "cert", + } { + if _, err := os.Stat(file); err != nil { + if os.IsNotExist(err) { + return errors.E(op, errors.Errorf("%s file '%s' does not exist", fileType, file)) + } + return errors.E(op, errors.Wrapf(err, "failed to check %s file", fileType)) + } + }
Line range hint
24-43: Consider using constants for default values.To improve maintainability and make it easier to update default values in the future, consider extracting the default values into constants.
+const ( + defaultGRPCAddress = "127.0.0.1:10000" + defaultProxyAddress = "tcp://127.0.0.1:30000" + defaultName = "roadrunner" + defaultVersion = "1.0.0" +) func (c *Config) InitDefaults() error { const op = errors.Op("centrifuge_init_defaults") if c.GrpcAPIAddress == "" { - c.GrpcAPIAddress = "127.0.0.1:10000" + c.GrpcAPIAddress = defaultGRPCAddress } if len(c.GrpcAPIAddress) > 7 && c.GrpcAPIAddress[0:6] == "tcp://" { c.GrpcAPIAddress = c.GrpcAPIAddress[6:] } if c.ProxyAddress == "" { - c.ProxyAddress = "tcp://127.0.0.1:30000" + c.ProxyAddress = defaultProxyAddress } if c.Name == "" { - c.Name = "roadrunner" + c.Name = defaultName } if c.Version == "" { - c.Version = "1.0.0" + c.Version = defaultVersion }
Line range hint
29-31: Improve GrpcAPIAddress prefix handling.The current implementation uses magic numbers and could be more robust. Consider using strings.TrimPrefix for better readability and maintainability.
- if len(c.GrpcAPIAddress) > 7 && c.GrpcAPIAddress[0:6] == "tcp://" { - c.GrpcAPIAddress = c.GrpcAPIAddress[6:] - } + const tcpPrefix = "tcp://" + c.GrpcAPIAddress = strings.TrimPrefix(c.GrpcAPIAddress, tcpPrefix)Don't forget to add the import:
+import "strings"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- config.go (1 hunks)
- schema.json (1 hunks)
🔇 Additional comments (1)
schema.json (1)
35-54: Verify external schema references are accessible.Let's ensure all external schema references are valid and accessible.
✅ Verification successful
External schema references are accessible and valid
All referenced schema URLs are accessible:
https://raw.githubusercontent.com/roadrunner-server/pool/refs/heads/master/schema.jsonhttps://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all external schema references are accessible echo "Checking external schema references..." # List of schema URLs to check urls=( "https://raw.githubusercontent.com/roadrunner-server/pool/refs/heads/master/schema.json" "https://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json" ) for url in "${urls[@]}"; do if curl --output /dev/null --silent --head --fail "$url"; then echo "✓ Schema accessible: $url" else echo "✗ Schema not accessible: $url" fi doneLength of output: 1047
| c.Pool = &pool.Config{} | ||
| c.Pool.InitDefaults() | ||
| } | ||
| c.Pool.InitDefaults() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for Pool initialization.
While moving c.Pool.InitDefaults() outside the nil check ensures defaults are always set, it might be worth handling any potential errors from the initialization.
if c.Pool == nil {
c.Pool = &pool.Config{}
}
-c.Pool.InitDefaults()
+if err := c.Pool.InitDefaults(); err != nil {
+ return errors.E(op, errors.Wrap(err, "failed to initialize pool defaults"))
+}Committable suggestion was skipped due to low confidence.
| { | ||
| "$id": "https://raw.githubusercontent.com/roadrunner-server/centrifuge/refs/heads/master/schema.json", | ||
| "$schema": "https://json-schema.org/draft/2019-09/schema", | ||
| "description": "All the valid configuration parameters for the Centrifugo plugin for RoadRunner.", | ||
| "type": "object", | ||
| "title": "roadrunner-centrifuge", | ||
| "additionalProperties": false, |
There was a problem hiding this comment.
Consider using versioned URLs for schema references.
The schema $id currently points to the master branch, which could lead to breaking changes if the schema is modified. Consider using a specific version tag or commit hash in the URL to ensure schema stability.
Apply this diff:
- "$id": "https://raw.githubusercontent.com/roadrunner-server/centrifuge/refs/heads/master/schema.json",
+ "$id": "https://raw.githubusercontent.com/roadrunner-server/centrifuge/v1.0.0/schema.json",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "$id": "https://raw.githubusercontent.com/roadrunner-server/centrifuge/refs/heads/master/schema.json", | |
| "$schema": "https://json-schema.org/draft/2019-09/schema", | |
| "description": "All the valid configuration parameters for the Centrifugo plugin for RoadRunner.", | |
| "type": "object", | |
| "title": "roadrunner-centrifuge", | |
| "additionalProperties": false, | |
| { | |
| "$id": "https://raw.githubusercontent.com/roadrunner-server/centrifuge/v1.0.0/schema.json", | |
| "$schema": "https://json-schema.org/draft/2019-09/schema", | |
| "description": "All the valid configuration parameters for the Centrifugo plugin for RoadRunner.", | |
| "type": "object", | |
| "title": "roadrunner-centrifuge", | |
| "additionalProperties": false, |
| "pool": { | ||
| "$ref": "https://raw.githubusercontent.com/roadrunner-server/pool/refs/heads/master/schema.json" | ||
| }, |
There was a problem hiding this comment.
Use versioned URL for pool schema reference.
The pool schema reference points to the master branch, which could lead to breaking changes.
Apply this diff:
"pool": {
- "$ref": "https://raw.githubusercontent.com/roadrunner-server/pool/refs/heads/master/schema.json"
+ "$ref": "https://raw.githubusercontent.com/roadrunner-server/pool/v1.0.0/schema.json"
},Committable suggestion was skipped due to low confidence.
| "tls": { | ||
| "description": "TLS settings", | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "cert": { | ||
| "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json#/$defs/SSL/properties/cert" | ||
| }, | ||
| "key": { | ||
| "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json#/$defs/SSL/properties/key" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "cert", | ||
| "key" | ||
| ] | ||
| } |
There was a problem hiding this comment.
Improve TLS configuration schema.
Two improvements needed for the TLS configuration:
- Use versioned URLs for SSL definition references
- Add descriptions for individual TLS properties
Apply these changes:
"tls": {
"description": "TLS settings",
"type": "object",
"additionalProperties": false,
"properties": {
"cert": {
+ "description": "Path to SSL certificate file",
- "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json#/$defs/SSL/properties/cert"
+ "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/v1.0.0/schema.json#/$defs/SSL/properties/cert"
},
"key": {
+ "description": "Path to SSL private key file",
- "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json#/$defs/SSL/properties/key"
+ "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/v1.0.0/schema.json#/$defs/SSL/properties/key"
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "tls": { | |
| "description": "TLS settings", | |
| "type": "object", | |
| "additionalProperties": false, | |
| "properties": { | |
| "cert": { | |
| "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json#/$defs/SSL/properties/cert" | |
| }, | |
| "key": { | |
| "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/refs/heads/master/schema.json#/$defs/SSL/properties/key" | |
| } | |
| }, | |
| "required": [ | |
| "cert", | |
| "key" | |
| ] | |
| } | |
| "tls": { | |
| "description": "TLS settings", | |
| "type": "object", | |
| "additionalProperties": false, | |
| "properties": { | |
| "cert": { | |
| "description": "Path to SSL certificate file", | |
| "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/v1.0.0/schema.json#/$defs/SSL/properties/cert" | |
| }, | |
| "key": { | |
| "description": "Path to SSL private key file", | |
| "$ref": "https://raw.githubusercontent.com/roadrunner-server/http/v1.0.0/schema.json#/$defs/SSL/properties/key" | |
| } | |
| }, | |
| "required": [ | |
| "cert", | |
| "key" | |
| ] | |
| } |
| "proxy_address": { | ||
| "description": "The address of the Centrifugo proxy server.", | ||
| "type": "string", | ||
| "default": "tcp://127.0.0.1:30000" | ||
| }, | ||
| "grpc_api_address": { | ||
| "description": "The address/port of the gRPC server API.", | ||
| "type": "string", | ||
| "default": "tcp://127.0.0.1:10000" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation patterns for network addresses.
The proxy_address and grpc_api_address properties lack pattern validation, which could lead to invalid configurations. Consider adding pattern validation to ensure valid TCP/Unix socket addresses and port ranges.
Add pattern validation:
"proxy_address": {
"description": "The address of the Centrifugo proxy server.",
"type": "string",
- "default": "tcp://127.0.0.1:30000"
+ "default": "tcp://127.0.0.1:30000",
+ "pattern": "^(tcp|unix)://[^\\s]+$"
},
"grpc_api_address": {
"description": "The address/port of the gRPC server API.",
"type": "string",
- "default": "tcp://127.0.0.1:10000"
+ "default": "tcp://127.0.0.1:10000",
+ "pattern": "^(tcp|unix)://[^\\s]+$"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "proxy_address": { | |
| "description": "The address of the Centrifugo proxy server.", | |
| "type": "string", | |
| "default": "tcp://127.0.0.1:30000" | |
| }, | |
| "grpc_api_address": { | |
| "description": "The address/port of the gRPC server API.", | |
| "type": "string", | |
| "default": "tcp://127.0.0.1:10000" | |
| }, | |
| "proxy_address": { | |
| "description": "The address of the Centrifugo proxy server.", | |
| "type": "string", | |
| "default": "tcp://127.0.0.1:30000", | |
| "pattern": "^(tcp|unix)://[^\\s]+$" | |
| }, | |
| "grpc_api_address": { | |
| "description": "The address/port of the gRPC server API.", | |
| "type": "string", | |
| "default": "tcp://127.0.0.1:10000", | |
| "pattern": "^(tcp|unix)://[^\\s]+$" | |
| }, |
| "version": { | ||
| "description": "Your application version.", | ||
| "type": "string", | ||
| "default": "v1.0.0" | ||
| }, | ||
| "name": { | ||
| "description": "Your application name.", | ||
| "type": "string", | ||
| "default": "roadrunner", | ||
| "minLength": 1 | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add semantic version validation pattern.
The version property should enforce semantic versioning format to ensure consistent version strings.
Add semantic version pattern validation:
"version": {
"description": "Your application version.",
"type": "string",
- "default": "v1.0.0"
+ "default": "v1.0.0",
+ "pattern": "^v(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "version": { | |
| "description": "Your application version.", | |
| "type": "string", | |
| "default": "v1.0.0" | |
| }, | |
| "name": { | |
| "description": "Your application name.", | |
| "type": "string", | |
| "default": "roadrunner", | |
| "minLength": 1 | |
| }, | |
| "version": { | |
| "description": "Your application version.", | |
| "type": "string", | |
| "default": "v1.0.0", | |
| "pattern": "^v(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$" | |
| }, | |
| "name": { | |
| "description": "Your application name.", | |
| "type": "string", | |
| "default": "roadrunner", | |
| "minLength": 1 | |
| }, |
fix missing pool default init
Reason for This PR
Added JSON schema.
Pool did not have its defaults set if you provided partial values.
Description of Changes
Added JSON schema.
Moved
c.Pool.InitDefaults()outside the init method.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes