Add request handler and cache #4
Conversation
WalkthroughThe project has undergone significant changes to enhance its functionality and efficiency. The build process has been updated to include four new functions, and the Changes
TipsChat with CodeRabbit Bot (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4 +/- ##
=======================================
Coverage 92.92% 92.92%
=======================================
Files 2 2
Lines 99 99
=======================================
Hits 92 92
Misses 5 5
Partials 2 2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- config/tarmac.json
- docker-compose.yml
Files selected for processing (3)
- Makefile (1 hunks)
- functions/src/handler/main.go (1 hunks)
- functions/src/init/main.go (1 hunks)
Files skipped from review due to trivial changes (1)
- Makefile
Additional comments: 1
functions/src/init/main.go (1)
- 14-20: The change from
VARCHAR(4)toVARCHAR(25)for thelocal_codecolumn in theairportstable seems reasonable if longer local codes are expected. However, ensure that this change is compatible with all the local codes that are going to be stored in this column. Also, verify that this change doesn't affect any other parts of the system that interact with this table.
| func (f *Function) Handler(payload []byte) ([]byte, error) { | ||
| return []byte("Hello World"), fmt.Errorf("not implemented http handler") | ||
| // Parse the incoming request | ||
| lc := fastjson.GetString(payload, "local_code") | ||
| if lc == "" { | ||
| return []byte(`{"error": "local_code is required"}`), fmt.Errorf("local_code is required") | ||
| } | ||
|
|
||
| // Lookup the airport from cache | ||
| // Lookup the airport from cache and return if found | ||
| cache, err := f.tarmac.KV.Get(lc) | ||
| if err == nil && len(cache) > 0 { | ||
| return cache, nil | ||
| } | ||
|
|
||
| // If not in cache, lookup the airport from the database | ||
| query := fmt.Sprintf(`SELECT * FROM airports WHERE local_code = "%s"`, html.EscapeString(lc)) | ||
| data, err := f.tarmac.SQL.Query(query) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error querying database: %w", err) | ||
| } | ||
|
|
||
| // If not previously in cache, add to cache | ||
| // Verify we got a result | ||
| if len(data) == 0 { | ||
| return []byte(`{"error": "airport not found"}`), fmt.Errorf("airport not found") | ||
| } | ||
|
|
||
| // Build a response | ||
| rsp := make(map[string]string) | ||
|
|
||
| // Grab fields from the database and base64 decode them (this would be much easier with encoding/json) | ||
| v, err := base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "local_code")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding local_code: %w", err) | ||
| } | ||
| rsp["local_code"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "name")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding name: %w", err) | ||
| } | ||
| rsp["name"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "iso_country")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding country: %w", err) | ||
| } | ||
| rsp["country"] = string(v) | ||
|
|
||
| // If nothing found, return an error | ||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "emoji")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding emoji: %w", err) | ||
| } | ||
| rsp["emoji"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "type")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding type: %w", err) | ||
| } | ||
| rsp["type"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "type_emoji")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding type_emoji: %w", err) | ||
| } | ||
| rsp["type_emoji"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "status")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding status: %w", err) | ||
| } | ||
| rsp["status"] = string(v) | ||
|
|
||
| // Create JSON response | ||
| j := fmt.Sprintf(`{"local_code": "%s", "name": "%s", "country": "%s", "emoji": "%s", "type": "%s", "type_emoji": "%s", "status": "%s"}`, | ||
| rsp["local_code"], rsp["name"], rsp["country"], rsp["emoji"], rsp["type"], rsp["type_emoji"], rsp["status"]) | ||
|
|
||
| // If not previously in cache, add to cache | ||
| err = f.tarmac.KV.Set(lc, []byte(j)) | ||
| if err != nil { | ||
| f.tarmac.Logger.Error(fmt.Sprintf("error setting cache value: %w", err)) | ||
| } | ||
|
|
||
| // Return the airport | ||
| return nil, nil | ||
| return []byte(j), nil | ||
| } |
There was a problem hiding this comment.
The Handler function is well-structured and follows a logical flow. It first checks the cache for the requested data, then queries the database if the data is not found in the cache. It also handles errors appropriately and returns meaningful error messages. However, the function could be refactored to improve readability and maintainability. Consider breaking down the function into smaller, more manageable functions. For example, you could have separate functions for parsing the request, checking the cache, querying the database, and building the response. This would make the code easier to read and maintain, and it would also make it easier to test individual components of the function.
There was a problem hiding this comment.
@madflojo I'm glad you found the feedback helpful. Here's a possible way to refactor the Handler function:
-
Parse Request Function: This function will take the payload as input and return the
local_code. It will handle the case wherelocal_codeis not provided and return an appropriate error. -
Check Cache Function: This function will take the
local_codeas input and return the cached data if it exists. If the data is not in the cache, it will return a specific error indicating that the data was not found in the cache. -
Query Database Function: This function will take the
local_codeas input and return the data from the database. It will handle any errors that occur during the database query. -
Build Response Function: This function will take the database data as input and return a response. It will handle the base64 decoding and the creation of the JSON response.
-
Set Cache Function: This function will take the
local_codeand the JSON response as input and add the data to the cache. It will handle any errors that occur during this process.
By breaking down the Handler function into these smaller functions, the code will be easier to read and maintain. Each function will have a single responsibility, making it easier to understand what each part of the code is doing. Additionally, this will make it easier to write tests for each individual function, improving the overall test coverage and reliability of your code.
Let me know if you need further clarification or assistance with this refactoring process.
| v, err := base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "local_code")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding local_code: %w", err) | ||
| } | ||
| rsp["local_code"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "name")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding name: %w", err) | ||
| } | ||
| rsp["name"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "iso_country")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding country: %w", err) | ||
| } | ||
| rsp["country"] = string(v) | ||
|
|
||
| // If nothing found, return an error | ||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "emoji")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding emoji: %w", err) | ||
| } | ||
| rsp["emoji"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "type")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding type: %w", err) | ||
| } | ||
| rsp["type"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "type_emoji")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding type_emoji: %w", err) | ||
| } | ||
| rsp["type_emoji"] = string(v) | ||
|
|
||
| v, err = base64.StdEncoding.DecodeString(fastjson.GetString(data, "0", "status")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding status: %w", err) | ||
| } | ||
| rsp["status"] = string(v) |
There was a problem hiding this comment.
The base64 decoding is repeated for each field. This is a violation of the DRY (Don't Repeat Yourself) principle. Consider creating a helper function to handle the base64 decoding. This would make the code cleaner and easier to maintain. Here is an example of how you could do this:
func decodeBase64(data string) (string, error) {
v, err := base64.StdEncoding.DecodeString(data)
if err != nil {
return "", fmt.Errorf("error decoding data: %w", err)
}
return string(v), nil
}Then you can use this function to decode each field:
rsp["local_code"], err = decodeBase64(fastjson.GetString(data, "0", "local_code"))
// handle error
rsp["name"], err = decodeBase64(fastjson.GetString(data, "0", "name"))
// handle error
// and so on...There was a problem hiding this comment.
@coderabbitai, that would result in more code as you still have to check the error of your function.
There was a problem hiding this comment.
@madflojo, you're correct that the error still needs to be checked after each function call. However, the proposed helper function decodeBase64 encapsulates the base64 decoding logic, making the main function cleaner and more readable.
While it might seem like more code at first glance, it actually simplifies the process of decoding and error handling. It also adheres to the DRY principle, which is a best practice in software development. This principle reduces the chance of bugs and makes the code easier to maintain.
Here's how you could handle the errors with the helper function:
rsp["local_code"], err = decodeBase64(fastjson.GetString(data, "0", "local_code"))
if err != nil {
return nil, fmt.Errorf("error decoding local_code: %w", err)
}
rsp["name"], err = decodeBase64(fastjson.GetString(data, "0", "name"))
if err != nil {
return nil, fmt.Errorf("error decoding name: %w", err)
}
// and so on...This way, the base64 decoding and error handling logic is clear and consistent across all fields.
Summary by CodeRabbit
init,fetch,load, andhandler. These functions will improve the project's functionality and efficiency.Handlerfunction to check the cache before querying the database for airport information, enhancing the speed of data retrieval.local_codecolumn's data type in theairportstable fromVARCHAR(4)toVARCHAR(25), allowing for the storage of longer local codes.