Complete GWI Favorites Service - Implementation

[adrfinance]

No description provided.

[pamanta]

Hi @adrfinance

Thank you very much for taking the time to complete the assignment!

I've left some questions for you to address, when you have the time. No rush, take your time. :)

[pamanta]

Could you identify some downsides of having a shared mutex for the repository? If yes, how would you handle those downsides?

[pamanta]

If favorites increase in size, this process could become a real bottleneck. Can you find a way to restructure the schema of the favorites repo to avoid it?

[pamanta]

Having a shared description between user favorites can be problematic? Can you identify why and how would you improve it?

[pamanta]

Identifying the user by userId passed in the path is fine for the assignment, but would that also work for a production environment? If not, how would you handle user identification differently?

[adrfinance]

Hi @pmantafounis-gwi, Many thanks for your feedback, please find my answers below. Looking forward to hearing from you

…

On Fri, Sep 5, 2025 at 12:26 PM Panagiotis Mantafounis < ***@***.***> wrote: ***@***.**** commented on this pull request. Hi @adrfinance <https://github.com/adrfinance> Thank you very much for taking the time to complete the assignment! I've left some questions for you to address, when you have the time. No rush, take your time. :) ------------------------------ In internal/repository/memory/repository.go <#36 (comment)> : > @@ -0,0 +1,267 @@ +package memory + +import ( + "sync" + "time" + + "gwi-favorites-service/internal/domain" + "gwi-favorites-service/internal/repository" +) + +// Repository implements FavoritesRepository using in-memory storage +type Repository struct { + mu sync.RWMutex Could you identify some downsides of having a shared mutex for the repository? If yes, how would you handle those downsides?

--------------- Looking at the shared mutex approach, I can identify several significant downsides: Key Downsides: 1. All operations (reads and writes) compete for the same lock, causing unnecessary blocking when multiple reads could safely execute concurrently 2. The entire repository is locked for any operation, preventing concurrent access to completely unrelated data 3. Scalability Bottleneck: Performance degrades significantly under high concurrency as operations are essentially serialized What I could do is use fine-grained locking with separate mutexes per user: type Repository struct { userMutexes sync.Map // map[string]*sync.RWMutex data sync.Map // map[string][]domain.Favorite} func (r *Repository) getUserMutex(userID string) *sync.RWMutex { mutex, _ := r.userMutexes.LoadOrStore(userID, &sync.RWMutex{}) return mutex.(*sync.RWMutex)} func (r *Repository) GetFavorites(userID string) ([]domain.Favorite, error) { mutex := r.getUserMutex(userID) mutex.RLock() defer mutex.RUnlock() favorites, exists := r.data.Load(userID) if !exists { return []domain.Favorite{}, nil } return favorites.([]domain.Favorite), nil} This approach provides: - User-level isolation: Operations on different users can run concurrently - Read concurrency: Multiple reads for the same user can execute simultaneously - Better scalability: Contention is limited to operations on the same user's data - Simple implementation: Leverages sync.Map for thread-safe mutex management This strikes the right balance between performance and implementation complexity for a favorites service where user-level isolation is the natural boundary.

------------------------------ In internal/repository/memory/repository.go <#36 (comment)> : > + // Update in all user favorites + for userID := range r.favorites { + if favorite, exists := r.favorites[userID][asset.GetID()]; exists { + favorite.Asset = asset + favorite.UpdatedAt = time.Now() + } + } If favorites increase in size, this process could become a real bottleneck. Can you find a way to restructure the schema of the favorites repo to avoid it?

-------------- Looking at this code, the bottleneck occurs because I am scanning all users to find who has favorited a specific asset. What I could do is add a reverse index that maps assets to the users who have favorited them type Repository struct { mu sync.RWMutex favorites map[string]map[string]*domain.Favorite // userID -> assetID -> favorite assetToUsers map[string]map[string]struct{} // assetID -> set of userIDs} func (r *Repository) AddFavorite(userID string, asset domain.Asset) error { r.mu.Lock() defer r.mu.Unlock() assetID := asset.GetID() // Add to user favorites if r.favorites[userID] == nil { r.favorites[userID] = make(map[string]*domain.Favorite) } favorite := &domain.Favorite{ Asset: asset, CreatedAt: time.Now(), UpdatedAt: time.Now(), } r.favorites[userID][assetID] = favorite // Update reverse index if r.assetToUsers[assetID] == nil { r.assetToUsers[assetID] = make(map[string]struct{}) } r.assetToUsers[assetID][userID] = struct{}{} return nil} func (r *Repository) updateAsset(asset domain.Asset) { r.mu.Lock() defer r.mu.Unlock() assetID := asset.GetID() // Only update users who actually have this asset favorited if userSet, exists := r.assetToUsers[assetID]; exists { for userID := range userSet { if favorite := r.favorites[userID][assetID]; favorite != nil { favorite.Asset = asset favorite.UpdatedAt = time.Now() } } }} Key Benefits: - O(1) lookup instead of O(all users) scan - Targeted updates only to users who actually have the asset - Maintains consistency between both indexes during add/remove operations - Significant performance improvement as the user base grows The trade-off is additional memory overhead for the reverse index, but this is minimal compared to the massive performance gain, especially as your user base scales.

------------------------------ In internal/service/favorites.go <#36 (comment)> : > +// UpdateFavoriteDescription updates the description of a favorite asset +func (s *FavoritesService) UpdateFavoriteDescription(ctx context.Context, userID, assetID, description string) error { Having a shared description between user favorites can be problematic? Can you identify why and how would you improve it?

------------------------ Yes, having a shared description between user favorites is highly problematic for several reasons: Key Problems: 1. User Privacy Violation: One user can see and modify descriptions written by other users 2. Unintended Overwrites: Users unknowingly overwrite each other's personal notes 3. No Personal Context: Users can't maintain their own private reasons for favoriting an asset 4. Confusing UX: Users expect their descriptions to be personal and persistent What I could do is make descriptions user-specific by moving them out of the shared Asset and into the user's Favorite record: type Favorite struct { Asset domain.Asset `json:"asset"` Description string `json:"description"` // User's personal description CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"`} type Asset struct { ID string `json:"id"` Name string `json:"name"` // Remove description from here - it belongs to the user's favorite CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"`} Updated Service Method: func (s *FavoritesService) UpdateFavoriteDescription(ctx context.Context, userID, assetID, description string) error { favorite, err := s.repo.GetFavorite(userID, assetID) if err != nil { return err } favorite.Description = description favorite.UpdatedAt = time.Now() return s.repo.UpdateFavorite(userID, assetID, favorite)} This approach ensures: - User isolation: Each user maintains their own private descriptions - No data conflicts: Users can't interfere with each other's notes - Better UX: Users get personalized, persistent descriptions - Proper data ownership: Personal annotations belong to the user, not the shared asset

------------------------------ In internal/handler/handler.go <#36 (comment)> : > + logger: logger, + } +} + +func (h *Handler) SetupRoutes() http.Handler { + r := mux.NewRouter() + + // API routes + api := r.PathPrefix("/api").Subrouter() + + // Apply middleware + api.Use(h.LoggingMiddleware) + api.Use(h.CORSMiddleware) + + // User favorites routes + userRoutes := api.PathPrefix("/users/{userID}/favorites").Subrouter() Identifying the user by userId passed in the path is fine for the assignment, but would that also work for a production environment? If not, how would you handle user identification differently?

------------------------- No, using userID in the path would not work for a production environment. This approach has serious security and usability issues: Key Problems: 1. Authorization Bypass: Any user can access any other user's favorites by changing the userID in the URL 2. Data Exposure: User IDs become visible in URLs, logs, and browser history 3. No Authentication: There's no verification that the requester is actually the user they claim to be 4. Enumeration Attacks: Attackers can easily guess/iterate through user IDs to access data My Recommendation: Use JWT-based authentication with user identification from the token: // Middleware to extract user from JWTfunc (h *Handler) AuthMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { token := r.Header.Get("Authorization") if token == "" { http.Error(w, "Missing authorization token", http.StatusUnauthorized) return } // Parse and validate JWT claims, err := h.jwtService.ValidateToken(token) if err != nil { http.Error(w, "Invalid token", http.StatusUnauthorized) return } // Add user ID to request context ctx := context.WithValue(r.Context(), "userID", claims.UserID) next.ServeHTTP(w, r.WithContext(ctx)) })} // Updated routesfunc (h *Handler) SetupRoutes() http.Handler { r := mux.NewRouter() api := r.PathPrefix("/api").Subrouter() api.Use(h.LoggingMiddleware) api.Use(h.CORSMiddleware) api.Use(h.AuthMiddleware) // Add auth middleware // Remove userID from path - get from token instead favoritesRoutes := api.PathPrefix("/favorites").Subrouter() favoritesRoutes.HandleFunc("", h.GetFavorites).Methods("GET") favoritesRoutes.HandleFunc("", h.AddFavorite).Methods("POST") return r} // Handler extracts userID from context instead of pathfunc (h *Handler) GetFavorites(w http.ResponseWriter, r *http.Request) { userID := r.Context().Value("userID").(string) // ... rest of handler logic} This approach provides: - Proper authentication: Only authenticated users can access the API - Automatic authorization: Users can only access their own data - Security: User IDs are not exposed in URLs - Standard practice: Follows OAuth 2.0/JWT patterns used in production systems

— Reply to this email directly, view it on GitHub <#36 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUMUVVUBFPHCOD4WMOPX5L3RFJNDAVCNFSM6AAAAACD4X5IW6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCOBYGUYDQMRUGE> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>