Implemented API call functionality in WebsiteBackend #68
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WebsiteBackend
participant HTTPClient
Caller->>WebsiteBackend: MakeAPICall(body)
WebsiteBackend->>WebsiteBackend: json.Marshal(body)
alt Marshaling Fails
WebsiteBackend-->>Caller: Return error and nil response
else
WebsiteBackend->>WebsiteBackend: Create HTTP Request
WebsiteBackend->>WebsiteBackend: PrepareHeaders(req)
WebsiteBackend->>HTTPClient: Execute HTTP Request
HTTPClient-->>WebsiteBackend: Return response
WebsiteBackend-->>Caller: Return response
end
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
utils/helper.go(2 hunks)utils/helper_test.go(2 hunks)
🔇 Additional comments (4)
utils/helper.go (2)
14-18: Good struct design with appropriate fields for API callsThe
WebsiteBackendstruct has a clean design with all necessary fields for making API calls. MakingAuthTokena pointer allows for optional authentication whileMethodandURLprovide the essential HTTP request information.
49-54: Authorization header implementation is secureThe
PrepareHeadersmethod correctly sets the Content-Type header and uses the secure Bearer token format for authorization when a token is provided.utils/helper_test.go (2)
112-135: Well-structured tests for PrepareHeadersThe tests for
PrepareHeadersare thorough, covering both scenarios with and without an auth token. The assertions effectively verify the expected header values.
91-92: Good use of unmarshalable type for error testingUsing a channel as test input is an excellent way to force a JSON marshaling error, as channels cannot be marshaled to JSON. This effectively tests the error handling path.
amit-flx
left a comment
amit-flx
left a comment
There was a problem hiding this comment.
why we are not adding any documentation, do we don't need this, is that design doc PRD ownership has been moved to RDS
it only includes ENV this env variable. And that is already mentioned in the .env.example and also in README. |
what about earlier created docs, please re-read whole line before replying half |
I've sent the request to transfer the ownership of the docs mentioned below to Let me know if there's anything missing. I can add that as well |
…nd update related tests
…ed methods and tests
| return err | ||
| } | ||
|
|
||
| func (hcp *HTTPClientService) prepareRequest(body interface{}) (*http.Request, error) { |
There was a problem hiding this comment.
any reason to use base data type not for going with struct or something else, base data come with certain added disadvantages
There was a problem hiding this comment.
The purpose of this function is to make API calls to external services, where request and response structures can vary widely. Binding specific types within the function would limit its flexibility and reusability. By using interface{}, the responsibility of type binding is delegated to the caller, ensuring the utility remains generic and adaptable across diverse use cases.
There was a problem hiding this comment.
If we can't determine contract structure no point of writing this
| return req, nil | ||
| } | ||
|
|
||
| func (hcp *HTTPClientService) readResponseBody(resp *http.Response, result interface{}) error { |
| return nil | ||
| } | ||
|
|
||
| func (hcp *HTTPClientService) MakeAPICall(body interface{}, result interface{}) error { |
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| type MockHTTPClient struct { |
There was a problem hiding this comment.
is this for unit test or integration test, I need clear segregation b/w unit test and integration test
There was a problem hiding this comment.
Did not understand what exact change you are asking for.
Since this file is a unit test file, should I rename this file and add unit in between? or what else is required here?
There was a problem hiding this comment.
for unit test why need to mock http client and where is code for integration tests
There was a problem hiding this comment.
it's making an api call. How do I test for success scenarios without mocking the server?
There was a problem hiding this comment.
is this part of unit test or integration test
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| URL: mockServer.URL, | ||
| } | ||
| originalTimeout := config.AppConfig.TIMEOUT | ||
| config.AppConfig.TIMEOUT = 2 |
There was a problem hiding this comment.
The timeout value is set to 2 without a time unit, which defaults to 2 nanoseconds. This is unrealistically short for an HTTP request test and will cause flaky test failures. Consider using a more appropriate duration like 2 * time.Millisecond or 50 * time.Millisecond to ensure consistent test behavior while still failing quickly when needed.
| config.AppConfig.TIMEOUT = 2 | |
| config.AppConfig.TIMEOUT = 50 * time.Millisecond |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
|
||
| t.Run("should return error if response does not matches with the provided dto", func(t *testing.T) { | ||
| result := &TestResponse{} | ||
| mockServer := MakeMockServer(`{success:false}`, http.StatusInternalServerError) |
There was a problem hiding this comment.
The JSON string is malformed and will cause parsing errors. Keys in JSON must be enclosed in double quotes. Please update to {"success":false} instead of {success:false}.
| mockServer := MakeMockServer(`{success:false}`, http.StatusInternalServerError) | |
| mockServer := MakeMockServer(`{"success":false}`, http.StatusInternalServerError) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| func (hcp *HTTPClientService) MakeAPICall(body interface{}, result interface{}) error { | ||
| req, err := hcp.prepareRequest(body) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| client := &http.Client{ | ||
| Timeout: config.AppConfig.TIMEOUT, | ||
| } | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| logrus.Errorf("Failed to make request: %v", err) | ||
| return err | ||
| } | ||
|
|
||
| defer resp.Body.Close() | ||
| return hcp.readResponseBody(resp, result) | ||
| } |
There was a problem hiding this comment.
The MakeAPICall method should validate HTTP status codes before processing the response. Currently, it attempts to parse the response body regardless of status code, which could lead to unexpected behavior with error responses (4xx/5xx). Consider adding status code validation to ensure only successful responses (2xx) are processed, while returning appropriate errors for other status codes:
resp, err := client.Do(req)
if err != nil {
logrus.Errorf("Failed to make request: %v", err)
return err
}
defer resp.Body.Close()
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return fmt.Errorf("API returned non-success status code: %d", resp.StatusCode)
}
return hcp.readResponseBody(resp, result)| func (hcp *HTTPClientService) MakeAPICall(body interface{}, result interface{}) error { | |
| req, err := hcp.prepareRequest(body) | |
| if err != nil { | |
| return err | |
| } | |
| client := &http.Client{ | |
| Timeout: config.AppConfig.TIMEOUT, | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| logrus.Errorf("Failed to make request: %v", err) | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| return hcp.readResponseBody(resp, result) | |
| } | |
| func (hcp *HTTPClientService) MakeAPICall(body interface{}, result interface{}) error { | |
| req, err := hcp.prepareRequest(body) | |
| if err != nil { | |
| return err | |
| } | |
| client := &http.Client{ | |
| Timeout: config.AppConfig.TIMEOUT, | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| logrus.Errorf("Failed to make request: %v", err) | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | |
| return fmt.Errorf("API returned non-success status code: %d", resp.StatusCode) | |
| } | |
| return hcp.readResponseBody(resp, result) | |
| } | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Date: 17 Mar 2025
Developer Name: Joy Gupta
Issue Ticket Number
N/A
Description
Added functionality to send user Discord data to an external API. This includes generating an authorization token, constructing the data payload, and making a POST request to the external API endpoint.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Test Coverage