Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes reorganize the authentication handling across the codebase. Function signatures in command and client modules now use the new Changes
Sequence Diagram(s)sequenceDiagram
participant T as Template
participant F as AuthFactory (NewAuth)
participant BA as BasicAuth
participant BE as BearerAuth
participant AA as ApiAuth
T->>F: Call NewAuth(template)
alt Authentication type is "basic"
F->>BA: Call NewBasicAuth(template)
BA-->>F: Return BasicAuth instance
else alt Authentication type is "bearer"
F->>BE: Call NewBearerAuth(template)
BE-->>F: Return BearerAuth instance
else alt Authentication type is "api"
F->>AA: Call NewApiAuth(template)
AA-->>F: Return ApiAuth instance
else
F-->>T: Return error (unknown type)
end
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/request/authentication/api.go (1)
18-39: NewApiAuth function implementation could be improved.The function correctly parses both JSON and YAML formats and validates required fields. However, there's a potential issue with nil handling:
Consider adding an explicit check for nil Auth before validating fields:
- if wrapper.Auth != nil && (wrapper.Auth.Header == "" || wrapper.Auth.Key == "") { + if wrapper.Auth == nil { + return nil, nil + } + + if wrapper.Auth.Header == "" || wrapper.Auth.Key == "" { return nil, errorsx.ErrApiAuthInvalid }This makes the logic more explicit and easier to follow, separating the nil check from the validation.
pkg/request/authentication/basic.go (1)
14-17: Add YAML tag to the struct for consistency.The
BasicAuthstruct properly has both JSON and YAML tags, but looking at the parsing logic, I notice that it attempts to unmarshal both JSON and YAML formats. For consistency, I recommend adding the YAML tag to maintain the same behavior across both formats.type BasicAuth struct { Username string `json:"username" yaml:"username"` Password string `json:"password" yaml:"password"` }pkg/request/authentication/bearer.go (2)
14-16: Add YAML tag to the Token field.The
BearerAuthstruct has a JSON tag but is missing a YAML tag for the Token field, even though the code attempts to unmarshal YAML in theNewBearerAuthfunction. For consistency and to ensure correct YAML parsing, add the YAML tag.type BearerAuth struct { - Token string `json:"token"` + Token string `json:"token" yaml:"token"` }
25-30: Inconsistent error handling pattern compared to basic.go.The error handling pattern in
bearer.godiffers frombasic.go. Inbasic.go, you're performing the YAML unmarshal outside the JSON error check conditional, while here you're nesting it within. Both achieve similar results, but for consistency and readability, consider using the same pattern across both files.err := json.Unmarshal(bs, &wrapper) if err != nil { - if err = yaml.Unmarshal(bs, &wrapper); err != nil { - return nil, fmt.Errorf("%w: %v", errorsx.ErrBearerAuthParse, err) - } + err = yaml.Unmarshal(bs, &wrapper) } +if err != nil { + return nil, fmt.Errorf("%w: %v", errorsx.ErrBearerAuthParse, err) }pkg/request/authentication/basic_test.go (1)
121-138: Consider adding a test for nil BasicAuth.Apply case.The current test verifies that Apply works correctly with a valid BasicAuth instance. Since the Apply method has a nil check (
if a != nil), it would be good to also test that calling Apply with a nil BasicAuth doesn't panic.func TestNilBasicAuthApply(t *testing.T) { var basic *BasicAuth = nil request, err := http.NewRequest(http.MethodGet, "", nil) assert.NotNil(t, request) assert.NoError(t, err) // This should not panic basic.Apply(request) // Verify no basic auth was set _, _, ok := request.BasicAuth() assert.False(t, ok) }pkg/request/authentication/authentication.go (1)
31-44: Good use of JSON→YAML fallback.The two-step unmarshal approach is valid. If performance becomes a concern or if there's a need for a single canonical representation, consider consolidating to a unified parsing method that detects the content type or merges both steps into one pass.
pkg/request/authentication/bearer_test.go (2)
60-60: Consider usingassert.Nilfor consistency.The assertion
assert.True(t, auth == nil)could be replaced withassert.Nil(t, auth)for consistency, as you're already usingassert.Nilin line 47 for a similar check.- assert.True(t, auth == nil) + assert.Nil(t, auth)
76-76: Consider flipping the order of parameters in assert.Equal for clarity.In most testing frameworks, the convention for assertions is
assert.Equal(t, expected, actual). Consider updating the assertion to follow this convention, which makes the test's intent clearer and error messages more readable.- assert.Equal(t, request.Header.Get("Authorization"), "Bearer helloworld") + assert.Equal(t, "Bearer helloworld", request.Header.Get("Authorization"))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/run.go(3 hunks)docs/json-schemas/request.schema.json(1 hunks)internal/errorsx/errorsx.go(1 hunks)pkg/client/client.go(2 hunks)pkg/request/authentication.go(0 hunks)pkg/request/authentication/api.go(1 hunks)pkg/request/authentication/api_test.go(1 hunks)pkg/request/authentication/authentication.go(1 hunks)pkg/request/authentication/authentication_test.go(1 hunks)pkg/request/authentication/basic.go(1 hunks)pkg/request/authentication/basic_test.go(1 hunks)pkg/request/authentication/bearer.go(1 hunks)pkg/request/authentication/bearer_test.go(1 hunks)pkg/request/authentication_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- pkg/request/authentication_test.go
- pkg/request/authentication.go
🧰 Additional context used
🧬 Code Definitions (8)
cmd/run.go (1)
pkg/request/authentication/authentication.go (2)
NewAuth(31-57)Auth(22-25)
pkg/client/client.go (3)
pkg/request/request.go (1)
Request(21-26)pkg/request/authentication/authentication.go (1)
Auth(22-25)pkg/result/result.go (1)
Result(13-22)
pkg/request/authentication/api.go (2)
pkg/request/authentication/authentication.go (3)
Auth(22-25)AuthType(14-14)Api(17-17)internal/errorsx/errorsx.go (2)
ErrApiAuthParse(7-7)ErrApiAuthInvalid(8-8)
pkg/request/authentication/basic.go (1)
pkg/request/authentication/authentication.go (3)
Auth(22-25)AuthType(14-14)Basic(18-18)
pkg/request/authentication/bearer.go (1)
pkg/request/authentication/authentication.go (3)
Auth(22-25)AuthType(14-14)Bearer(19-19)
pkg/request/authentication/api_test.go (1)
pkg/request/authentication/api.go (2)
NewApiAuth(18-39)ApiAuth(13-16)
pkg/request/authentication/authentication.go (3)
internal/errorsx/errorsx.go (2)
ErrAuthParse(6-6)ErrAuthUnknown(5-5)pkg/request/authentication/basic.go (1)
NewBasicAuth(19-40)pkg/request/authentication/bearer.go (1)
NewBearerAuth(18-37)
pkg/request/authentication/authentication_test.go (1)
pkg/request/authentication/authentication.go (3)
NewAuth(31-57)Basic(18-18)Bearer(19-19)
🔇 Additional comments (19)
internal/errorsx/errorsx.go (1)
7-8: Error definitions for API authentication look good.The new error variables for API authentication follow the existing pattern in the file. The error messages are clear and descriptive, handling parsing failures and validation requirements appropriately.
cmd/run.go (3)
12-12: Import for new authentication package added correctly.The import for the dedicated authentication package has been added, which aligns with the architectural changes separating authentication logic into its own package.
37-37: Authentication creation updated to use new package.The code now properly uses
authentication.NewAuth()instead of the previous implementation, maintaining the same functionality while using the reorganized structure.
51-51: Function signature updated to use the new Auth type.The parameter type has been correctly changed from
request.Authtoauthentication.Authto reflect the new package structure, ensuring type consistency throughout the codebase.pkg/client/client.go (2)
10-10: Import for new authentication package added correctly.The import for the dedicated authentication package has been properly added, consistent with the architectural changes in the authentication system.
34-34: Method signature updated to use the new Auth type.The parameter type has been correctly changed from
request.Authtoauthentication.Auth, maintaining functionality while reflecting the new package structure.pkg/request/authentication/api.go (3)
13-16: ApiAuth struct defined with appropriate fields.The struct definition with Header and Key fields is properly tagged for both JSON and YAML unmarshaling, making it compatible with both formats as required by the application.
41-43: Apply method implementation is clean and straightforward.The implementation properly sets the request header using the Header and Key fields, which is the standard way to add API key authentication to an HTTP request.
45-47: GetType method correctly returns the authentication type.The method returns the Api constant as expected for the Auth interface implementation.
pkg/request/authentication/basic.go (3)
19-40: LGTM! Robust error handling and validation.The
NewBasicAuthfunction correctly attempts to unmarshal as JSON first, then falls back to YAML. It also includes proper validation for required fields and returns appropriate errors.
42-46: LGTM! Properly handles nil case.The
Applymethod includes a nil check before attempting to set the authentication, which prevents potential nil pointer dereferences.
48-50: LGTM! Correctly implements the Auth interface.The
GetTypemethod returns the appropriate authentication type constant.docs/json-schemas/request.schema.json (1)
11-28: LGTM! Well-structured API authentication schema.The new API authentication schema is correctly defined with all required properties and follows the same structure as the existing authentication types. The schema properly requires the "type", "header", and "key" fields, which are all essential for API key authentication.
pkg/request/authentication/bearer.go (2)
39-41: LGTM! Correctly implements the Auth interface.The
GetTypemethod returns the appropriate authentication type constant.
43-47: LGTM! Properly handles nil case and sets Authorization header correctly.The
Applymethod includes a nil check before setting the header and properly formats the Bearer token in the Authorization header.pkg/request/authentication/basic_test.go (1)
12-119: LGTM! Comprehensive test cases for BasicAuth creation.The test covers all important scenarios for creating a BasicAuth instance, including successful creation from both YAML and JSON, handling invalid templates, missing auth fields, and validation of required fields.
pkg/request/authentication/api_test.go (1)
1-83: LGTM!All test scenarios for
ApiAuthappear solid and sufficiently validated.pkg/request/authentication/bearer_test.go (2)
11-62: Well-structured tests with thorough coverage.The
TestNewBearerAuthfunction is well-organized with multiple test cases covering various scenarios: YAML format, JSON format, invalid template, and template without auth. This comprehensive approach ensures robust functionality of the Bearer authentication implementation.
64-77: Well-implemented test for BearerAuth.Apply method.The
TestBearerAuthApplyfunction properly validates that the Bearer authentication is correctly applied to the HTTP request header.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/request/authentication/authentication_test.go (1)
11-39: Good test coverage for error casesThe tests properly verify error handling for unknown authentication types in both YAML and JSON formats.
However, there's a trailing comma in the JSON at line 30 which could cause parsing issues in some JSON parsers:
- "type": "unknown", + "type": "unknown"While this might be caught by the JSON parser and handled properly through the error path, it's better to use valid JSON in tests to avoid confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/request/authentication/authentication.go(1 hunks)pkg/request/authentication/authentication_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pkg/request/authentication/authentication.go (4)
internal/errorsx/errorsx.go (2)
ErrAuthParse(6-6)ErrAuthUnknown(5-5)pkg/request/authentication/api.go (1)
NewApiAuth(18-39)pkg/request/authentication/basic.go (1)
NewBasicAuth(19-40)pkg/request/authentication/bearer.go (1)
NewBearerAuth(18-37)
🔇 Additional comments (8)
pkg/request/authentication/authentication.go (5)
14-20: Clear enum definition for authentication typesThe
AuthTypeenum is well-defined with support for API, Basic, and Bearer authentication types. This provides a good foundation for the authentication framework.
22-25: Concise and well-designed Auth interfaceThe
Authinterface is clean and focused with just two methods:Applyto modify the HTTP request with authentication details, andGetTypeto identify the authentication type. This makes the interface easy to implement and use.
27-29: Clean struct definition with proper tagsThe
AuthBasestruct properly includes JSON and YAML tags for serialization compatibility, which ensures proper marshaling/unmarshaling in both formats.
31-47: Well-implemented authentication parsing with error handlingThe
NewAuthfunction handles both JSON and YAML formats with appropriate error handling. The check fornilauth is a good defensive programming practice that prevents nil pointer dereferences.
49-59: Complete switch statement with API authentication supportThe switch statement now includes all three authentication types, including the API authentication type that was missing in previous versions according to past review comments.
pkg/request/authentication/authentication_test.go (3)
41-52: Appropriate handling of no-auth caseThe test for the "no auth" scenario properly verifies that when no authentication information is provided, the function returns
nilwithout an error, which is the expected behavior.
54-81: Comprehensive tests for Basic and Bearer authenticationThe tests for Basic and Bearer authentication thoroughly validate the correct instantiation of the respective auth types. The assertions check that no error is returned, the auth object is not nil, and the authentication type is correctly identified.
83-96: Test coverage added for API authenticationAs requested in previous review comments, test coverage has been added for the API authentication type. The test validates that the correct authentication object is created with the expected type. This completes the test coverage for all supported authentication types.
1 participant
Summary by CodeRabbit