GWI Go Platform Challenge Submission

[Mary-LP]

Dear GWI team,

This PR contains my submission for the GWI Platform Go Challenge.

• All endpoints, usage examples. testing instructions and design notes are documented in README.md.
• Docker and Compose workflows included.
• Tests pass (go test ./... -v).

Thank you for reviewing! Please do not hesitate to contact me with comments, concerns or requests for alterations.

Kind regards,
Mary Loukidi-Papanikoli

P.S. I opted to do the development privately and only push the completed project. This is the exception, not the rule.

[NikosMas]

Hello Mary 👋

Thank you for your time and for the amount of work you’ve done!
I really liked several aspects of your approach on the project:

• The overall project structure and packaging
• The error handling using common error types
• The extensive test coverage you added
• The way you handled reads/writes in the storage layer (dictionaries) with mutexes
• The fact that you added an authorisation flow

I’ve left some comments, and I also have a few questions:

1. You mentioned that you would use PostgreSQL if you chose to persist the data. Why PostgreSQL and not a NoSQL database?
2. Regarding logging, where would you add logs and why?
3. Suppose I add a favourite asset and then cancel the request almost immediately. How do you (or how would you) handle such cases—e.g., ensuring consistency and dealing with partial or cancelled operations?

Thanks again!

[NikosMas]

This error will lead to a 500 error response to the user. Same error here in the RemoveFavourite method returns 404 error response. Which approach works better for you?

General question:
When would you prefer using the 500 and when the 4** error responses?

[Mary-LP]

Let me start by mentioning that if there was anything that I would have liked to spend more time on it would be error handling; admittedly it has been inconsistent and this is one example that escaped my proofreading. Ideally I would create a method with a switch statement that would consistently return a certain error code according to the error type.

Now to answer your question: I would use the 500 error code for server-side errors (unexpected failures like logic bugs, database crashes, unhandled exceptions etc.) and 4xx codes for client-side errors (bad request, unauthorized etc.). In retrospect I would have changed this to return ErrAssetNotFound like in RemoveFavorite and the handler to respond with 404.

[NikosMas]

This error here will lead to a 500 error response to the user. Is there any other status code that could be used for this kind of error?

[Mary-LP]

Yes, in retrospect I would return a 409 error response; the client is trying to create a resource that already exists, which conflicts with the current state.

[NikosMas]

Lines 36 to 49 are duplicated for every method in the asset handler. Also, this pattern, with the error mapping to HTTP errors, is used in all handlers. Can you think of a better approach to avoid the duplication?

[Mary-LP]

As an immediate improvement, I would use a method that takes the error code as input and responds appropriately. It would look like this:

func WriteJSONError(w http.ResponseWriter, message string) {
	statusCode := MapErrorToStatusCode(message)
	errorResponse := ErrorResponse{Error: message}
	w.Header().Set("Content-Type", "application/json")
	w.WriteHeader(statusCode)
	if err := json.NewEncoder(w).Encode(errorResponse); err != nil {
		http.Error(w, "Failed to encode error response", http.StatusInternalServerError)
	}
}

MapErrorToStatusCode would essentially be a bit switch statement that would map all the errors defined in errors.go to an http status code.

For a more long-term approach that is slightly more verbose but also type-safe is to have typed errors that wrap around the error type, which would look like:

type AppError struct {
    Code       string
    Message    string
    StatusCode int
    Err        error // underlying error for wrapping
}

and an example usage would be:

var (
    ErrInvalidUserID = &AppError{
        Code:       "INVALID_USER_ID",
        Message:    "invalid user id",
        StatusCode: http.StatusBadRequest,
    }
    ErrUserNotFound = &AppError{
        Code:       "USER_NOT_FOUND",
        Message:    "user not found",
        StatusCode: http.StatusNotFound,
    }
)

WriteJSONError would also be simpler, not to mention we would be avoiding a giant switch statement.

[NikosMas]

What's the difference in using a string type?

[Mary-LP]

I opted to use custom unexported types instead of string because it is recommended by the context package docs since they provide:

• Collision avoidance: If I used plain string, any package could use the same "userID" string and create collisions. With my chosen approach even if another package defines type ctxKey string with the same constants, Go's type system would treat them as different types.
• Encapsulation: ctxKey is unexported, so external packages can't create their own ctxKey values; they must use UserID and Role.

[NikosMas]

Are there any other types of groups, and why did you choose the waitgroup?

[Mary-LP]

I had to do a bit of research to provide a more complete response. There are definitely other types of groups like channels, errgroup.Group, errgroup.WithContext, context with channels, and semaphores. Most - if not all - of the ones I just mentioned support error handling and cancellation. Both sync.WaitGroup and errgroup.Group/errgroup.WithContext are low-complexity options, with errgroup.* having the error handling and cancellation advantage over sync.WaitGroup. However, I still opted for sync.WaitGroup because:

• We just need to wait for goroutines to finish.
• There are no errors to collect (test failures are handled by t.Errorf).
• There is no need for cancellation in a unit test.
• It is part of the standard library, so there are zero dependencies.

I would use the alternatives in the following scenarios:

• Production code with errors: errgroup.Group or errgroup.WithContext
• Need for cancellation: context + channels or errgroup.WithContext
• Rate limiting: semaphore.Weighted
• Result collection: channels

[ba091724]

Hello @Mary-LP,

Thank you for taking the time to design, implement, and nicely document the assignment!

I have added a couple of comments for you to look when you have the time.

I would also like to get your opinion regarding the following scenario:
Let's consider a case where the application database hosts a large amount of assets, persisted under an entity table named Assets. How would you adapt your implementation to support/enhance the functionality of user favorites? Would you change the way user favorites are handled (created/updated/deleted)?

[ba091724]

How would you handle a real-world application where the web client (i.e. the company website) needs to receive the JWT?

[Mary-LP]

To be fully honest, I personally do not have a lot of exposure to the authentication side of applications so I had to do some quick research to answer this. The method I narrowed it down to - assuming the company website is an SPA - is using HTTP-only cookies; they cannot be accessed via JavaScript and they help protect against XSS attacks. I would also enhance this with a refresh token - also stored in an HTTP-only cookie, separate from the JWT - to be used to request a new JWT when the previous one expires.

At the end of this evaluation process I would appreciate it if you could provide your own thoughts and/or preferred methods.

[ba091724]

Your research is valid, yes. Personally, and considering the scope of the assignment, I think it suffices to mention that including the JWT in the response body is a simplicity-driven decision and not an option in a real-world application. IMHO the Set-Cookie response header is ok for this context.

[ba091724]

Can you spot one or more weak points regarding this endpoint (API design, implementation)?

[Mary-LP]

I can spot a few issues:

• All the endpoints are slightly verbose and are inconsistent with REST conventions, where the ID should be in the URL.
• They are harder to cache/log/audit since the IDs are in the query string.
• No OPTIONS or HEAD support, which may break CORS
• No rate limiting; DDoS vulnerability
• No request size limits; huge JSON payloads could exhaust memory
• No logging or tracing; harder to debug production issues

As a step-one improvement, I would rework these lines as follows:

api.HandleFunc("/favorites", favoritesHandler.list).Methods("GET")             // List
api.HandleFunc("/favorites", favoritesHandler.add).Methods("POST")             // Add
api.HandleFunc("/favorites/{id}", favoritesHandler.update).Methods("PATCH")    // Update
api.HandleFunc("/favorites/{id}", favoritesHandler.remove).Methods("DELETE")   // Delete

Adding OPTIONS or HEAD support, rate and size limiting, and logging/tracing is another can of worms I cannot reasonably address in a comment.

[Mary-LP]

Hi @NikosMas,

Thank you for your comments and thought-provoking questions. I hope my answers to your comments are sufficient and, if not, please do not hesitate to follow up.

As for your questions:

1. You mentioned that you would use PostgreSQL if you chose to persist the data. Why PostgreSQL and not a NoSQL database?

Purely for familiarity reasons; I have used PostgreSQL in the past and would know how to handle and set it up with a decent amount of reliability. I would not be opposed to a NoSQL database and it may indeed a better solution, but I would need more time for research.

2. Regarding logging, where would you add logs and why?

There are plenty of places I would (and should) have added logs. Some of them are:

• Auth middleware for security auditing, to track auth failures, invalid tokens, missing credentials
• Auth, to audit token issuance and to detect token tampering
• All handlers, to trace request flow, measure latency, correlate with errors
• Admin operations for compliance and security, to track privileged actions
• Storage layer, to detect data corruption, concurrency issues and unexpected states
• Config, to detect misconfiguration (like using default secrets in prod)
• Tests, to debug flaky tests and understand execution flow for more complicated tests

3. Suppose I add a favourite asset and then cancel the request almost immediately. How do you (or how would you) handle such cases—e.g., ensuring consistency and dealing with partial or cancelled operations?

What I would ideally wish to do is not the current behavior:

• Currently, when a client cancels a request the handler continues executing unbothered, so (assuming no errors) the asset is saved in memory. The response write in that case will fail silently; json.NewEncoder(w).Encode() will return a broken pipe error, but the error value is not checked itself. The whole behavior is not great, but it would be especially catastrophic in deletions.
• As a step up (or several steps up honestly), I would be passing the context to all storage methods and would be checking ctx.Done() before and after acquiring locks. I would also be using idempotency key support for storage mutation endpoints to ensure that retries are safe. Finally, if I had a database which is a given in a real scenario, I would make these operations a two-phase commit, where I would commit to the database only if the context is still active.
• Ideally, I would use a Redis Cache for session management, rate limiting, idempotency, and of course for caching API responses.

[Mary-LP]

Hi @ba091724,

Thank you very much for your comments. I hope I have answered them sufficiently and, if not, please do not hesitate to follow up. Especially for your first one I will admit again some unfamiliarity with authentication and token handling.

Now for your question:

Let's consider a case where the application database hosts a large amount of assets, persisted under an entity table named Assets. How would you adapt your implementation to support/enhance the functionality of user favorites? Would you change the way user favorites are handled (created/updated/deleted)?
To start, having an Assets table makes perfect sense for a real world scenario and in retrospect I should have tried to simulate this more efficiently. Having the Assets table would save a lot of storage space, especially in scenarios where users can share favorites.

As I mentioned to Nikos, I am more familiar with relational databases and approaches, so my answer is going to be geared around that. I would approach this case as follows:

• Instead of having a map of user IDs to a map of asset IDs to assets I would use a user ID + asset ID junction table. I would also be adding some metadata information to that table (per-user created and updated timestamps, and a notes or description section - is this what the description that is mentioned in the task README supposed to be?)
• The concept of asset ownership could also be added (a user ID foreign key column in the Asset table). This would also differentiate asset creation/update/deletion from favorite creation/update/deletion; only an "asset owner" could update or delete the actual asset.
• Creating a favorite would be simplified to storing a user ID + asset ID reference in the junction table.
• Updating a favorite would be limited to metadata editing.
• Deleting a favorite would be simplified to removing the user ID + asset ID reference and the accompanying metadata from the junction table
• Getting a list of a user's favorites is going to be one extra read from the database (accessing the junction table to get the correct ID pairs and then the assets table to get the corresponding assets), but that could be optimized by adding a caching layer. A metadata-only list could also be an option.

To briefly elaborate on the asset handling assuming the concept of asset ownership that I mentioned above:

• Asset creation is pretty much the same in terms of the amount of information stored. A flag to also mark a newly-created asset as its owner's favorite could be added.
• An asset could have a list of owners (probably another junction table). New owners must be added by existing owners.
• Asset updates can only performed by their owners. For multiple asset owners, notifications to the other owners could be sent.
• Assed deletions can only be performed by their owners. Like updates with multiple aset owners, notifications to the oter owners could be sent.
• Notifications for asset updates/deletions could be sent to the users that have these assets as favorites, but bulk actions like that could easily overburden the system.
• If multiple asset owners is a thing, perhaps an "asset admin" (the original creator) should exist. Then we may have to worry about transferring admin rights, having the admin confirm asset changes/deletions, and other similar transactions. But this is a completely different rabbit hole, which I doubt is pertinent to the matter at hand. Still, let me know if you would like me to attempt to go down said rabbit hole.

I hope this answers your question. Please let me know if you have any further comments or questions!