-
Notifications
You must be signed in to change notification settings - Fork 0
Implement user favourites service (Go challenge) #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
pmantafounis-gwi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @parath, thank you very much for taking the time to complete the challenge. Overall, your implementation was clear, and easy to follow!
I've added some comments on my side, for you to take a look when you have the time :)
| RUN go build -o favourites main.go | ||
|
|
||
| # Runtime stage | ||
| FROM alpine:3.22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 🙏
| r.HandleFunc("/favourites/{userId}", s.getFavouritesHandler).Methods("GET") | ||
| r.HandleFunc("/favourites/{userId}", s.addFavouriteHandler).Methods("POST") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having {userId} as a resource identifier in favourites feels ambiguous. How could you restructure the endpoints to convey the ownership better and make it a bit more restful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, using {userId} in the path is ambiguous. A more RESTful design would be /users/{userId}/favourites, which makes ownership explicit. In a real system I would also rely on authentication/authorization (e.g. JWT) to derive the user context, which would allow simplifying to /favourites.
| f.ID = s.store.NextFavouriteID() | ||
| created, err := s.store.AddFavourite(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you spot any issues with calling s.store.NextFavouriteID() before s.store.AddFavourite(f)? If yes how would you solve them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Calling NextFavouriteID() separately could potentially cause inconsistencies under concurrency or service failures.
A safer approach would be to generate the ID inside AddFavourite so the operation is atomic. In a production setup with a database, I would let the DB assign IDs (via sequences/UUIDs) to ensure consistency.
| func writeError(w http.ResponseWriter, r *http.Request, status int, msg string, code ErrorCode) { | ||
| logRequest(r, fmt.Sprintf("status=%d code=%s msg=%s", status, code, msg)) | ||
| writeJSON(w, status, apiError{Error: msg, Code: code}) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other ways for logging the requests without having to always use this custom function across the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes there are other ways, while the helper works, a standardized solution would be the preferred approach.
Middleware-based logging or a structured logging library like zap or logrus would avoid custom calls across handlers and give more consistent logs.
| ## Assumptions | ||
| - REST API endpoints to fetch, add, remove and update assets in favourites list. | ||
| - JSON request/response with lower-camel JSON keys (id, userId, assetId, assetType, description, assetData, createdAt, updatedAt). | ||
| - In-memory store for the challenge purposes. No data store persistence present. Stored data are lost on restart. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you model it in a database? What DB would you use and with what schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For persistence, I would align with the platform’s existing DB solution.
Assuming Postgres, I would model it like:
CREATE TABLE favourites (
id TEXT PRIMARY KEY,
user_id TEXT NOT NULL,
asset_id TEXT NOT NULL,
asset_type TEXT NOT NULL,
description TEXT,
asset_data JSONB NOT NULL,
created_at TIMESTAMP NOT NULL,
updated_at TIMESTAMP NOT NULL,
UNIQUE(user_id, asset_id)
);JSONB gives flexibility for asset data while enabling indexing.
The unique (user_id, asset_id) constraint enforces the same rule as in the in-memory store.
lkslts64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parath Thanks for submitting the Go challenge, we really appreciate your effort. You delivered a clean and pragmatic solution featuring easy-to-read documentation and robust test coverage.
Below, I have prepared some questions for you to check
| AssetID string `json:"assetId"` | ||
| AssetType AssetType `json:"assetType"` | ||
| Description string `json:"description,omitempty"` | ||
| AssetData json.RawMessage `json:"assetData"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What’s the reason behind opting for json.RawMessage type here instead of []byte ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! I chose json.RawMessage because it makes the intent explicit: the field holds JSON. It avoids extra (un)marshalling steps compared to []byte when embedding JSON blobs and integrates directly with Go’s encoding/json.
| func main() { | ||
| r := httpapi.NewServer(favourites.NewInMemoryStore()) | ||
| log.Println("Server listening on :8080") | ||
| log.Fatal(http.ListenAndServe(":8080", r)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.Fatal is used to terminate the server immediately when the process receives a signal. How would you modify your server to shutdown gracefully? Why graceful shutdowns matters in a Kubernetes environment? And finally, how would a non-graceful server termination impact in-flight requests that write to the in-memory store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! Graceful shutdowns are important in Kubernetes where pods are terminated during scaling or updates. Without it, in-flight requests could be dropped and writes to the in-memory store lost.
In the current solution I use http.ListenAndServe, but for production I would wrap this in an http.Server and handle Shutdown(ctx) to catch OS signals and let ongoing requests finish cleanly.
| ) | ||
|
|
||
| func main() { | ||
| r := httpapi.NewServer(favourites.NewInMemoryStore()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you were to add some HTTP middleware to this app, what would that be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first I would add is structured request logging. Authentication/authorization middleware would also be essential once the service is integrated into the platform.
|
|
||
| // InMemoryStore stores favourites in memory per user | ||
| type InMemoryStore struct { | ||
| mu sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see that you use a RW mutex 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 Thanks!
|
@pmantafounis-gwi @lkslts64 Thanks a lot for the thoughtful review. 🙏 This was a great exercise and I really appreciate the feedback, as it helped me reflect on both design and production aspects. |
Adds an implementation of user favourites with REST API and tests.