GWI Platform Go Challenge by mr-polymath · Pull Request #49 · GlobalWebIndex/platform-go-challenge

mr-polymath · 2025-10-20T17:21:57Z

No description provided.

…d seed data. Add basic authorization mechanism. Add basic tests.

pamanta

Thank you very much for taking the time to complete the assignment.
Overall your submission was easy to follow covering plenty of the challenge aspects with a nice touch with swagger, good job!!

I've left some comments for you to have a look when you have the time, no rush :)

pamanta · 2025-10-30T15:00:33Z

@@ -0,0 +1,21 @@
+# Use the official Golang image
+FROM golang:latest


Could using the latest image of golang have any drawbacks? If yes how would you handle the image creation in a production environment

In production, one should use a specific version of any image and not any tag that changes over time. In this case, using latest may result in new bugs, vulnerabilities and new breaking changes. In addition, behaviour and bug reports will be inconsistent across different clients.

What should have been done in this case is pin the version to the current latest (i.e. 1.25 or 1.25.3) instead of using the "latest" tag.

pamanta · 2025-10-31T09:14:07Z

+The project uses swaggo/swag to generate Swagger documentation. To regenerate the docs run the following command from the repository root:
+
+```bash
+$(go env GOPATH)/bin/swag init -g main.go -d cmd/app,internal/api,internal/model --parseInternal -o internal/swagger


Can you find of any other way of adding swag without requiring the user to install it manually?

Running "go run github.com/swaggo/swag/cmd/swag@" instead of "$(go env GOPATH)/bin/swag init" would arguably be better for a more centralised way, it would not require any manual installation and it would be easier for deployment among teams.

pamanta · 2025-10-31T09:14:30Z

+	r.Use(gzip.Gzip(gzip.BestCompression)) // Enable gzip compression
+
+	// Swagger UI route
+	r.GET("/swagger/*any", ginSwagger.WrapHandler(swaggerFiles.Handler))


pamanta · 2025-10-31T09:16:51Z

+	userId, err := uuid.Parse(userIdFromToken)
+	if err != nil {
+		c.JSON(http.StatusNotFound, gin.H{"error": "user not found"})


Could you please explain the rationale behind returning a StatusNotFound on uuid parsing errors?

On second thought, this is a 400 BadRequest "invalid ID" situation and not a 404 NotFound "user not found" one. The current error is incorrect, because a parsing error does not indicate a missing resource, but a client error (in this case, an invalid input).

pamanta · 2025-10-31T11:37:04Z

+		return
+	}
+
+	c.JSON(http.StatusNoContent, gin.H{"message": "description updated"})


Do you see anything confusing about this response?

A 204 NoContent with content is an oxymoron, ergo it should either return 200 StatusOK with (preferably) the resource in its current state rather than a simple message, or not have a body at all (c.JSON(http.StatusNoContent, nil)). I generally prefer 204 for updates, but this depends on the use case and the team's policy.

lkslts64

Hey @mr-polymath, thanks for submitting the assignment! You did really well, showcasing a solid REST API Design with decent code quality 🥇 . You also added some nice extras, like Authentication, Swagger, Docker and tests 💯

I have left a couple of comments for you to check when you find some free time ☺️

lkslts64 · 2025-10-31T11:48:16Z

+	}
+
+	// Sorting by q.Order or ID by default, because map does not
+	// guarantee the order  of iteration


Great job 💯

lkslts64 · 2025-10-31T11:52:33Z

+type MemoryStore struct {
+	mu sync.RWMutex
+	// map[userID]map[assetID]Asset
+	favourites map[uuid.UUID]map[uuid.UUID]model.Asset


Could you briefly explain what are the pros and cons when designing your store with the above nested map versus map[uuid.UUID]model.Asset ?

map[userID]map[assetID]Asset (what was ultimately used)

Pros:

O(1) complexity for lookup, update and delete

O(1) complexity for accessing all user's assets

Guarantees uniqueness (no duplicates) of user-assets relationship

Better scalability for many users and assets (although this is an in-memory solution, therefore it might be a moot point)

Can be easily and quickly paginated for each user

Cons:

O(n) complexity when getting all assets for a user

Technically a bit more complicated than other solutions

Needs ordering (maps do not guarantee order of iteration)

Slight memory overhead due to additional key

No easy way to lookup a specific asset

map[assetID]Asset (I assume the question was not about map[userID]Asset, as that conflicts with the challenge's requirements)

Pros:

O(1) complexity for a specific asset lookup

Simpler

Less memory overhead

Cons:

O(n) complexity when getting all assets for a user (asset needs no store userID internally and would need to iterate through all assets and filter by userID)

Unclear data separation between users and their assets

Needs ordering

No efficient pagination (every query requires iterating through assets and filtering by user every time)

Does not scale for many users

map[userID][]Asset (a map of slices, another contender, my initial implementation and what I first thought/read you asked)

Pros:

O(1) complexity for getting all assets for a user

Simpler than the nested map solution

Does not need ordering - slices preserve order

Cons:

O(n) complexity for lookup, update and delete

Possibility of duplicate entries in the slice (it is not enforced by a key, like in maps)

Deletion requires creating a new slice

Does not scale for many assets

lkslts64 · 2025-10-31T11:57:08Z

+		return
+	}
+
+	var req patchReq


Currently, the path request body only accepts the "Description" attribute, which is fine. If you were to add support for the "Data" attribute, can you explain, in a high-level way, how you would implement that? For example, would the Asset.Data type remain any or would you change that?

I decided to use "Data any" mainly for simplicity. The better way is to provide a strongly-typed data type. This can be achieved with a struct (AssetData, for example) that contains pointers to each asset data (*ChartData, *InsightData and *AudienceData, in this case). This gives type safety, predictable JSON schemas, and better Swagger docs and validation.

Another change would be extending the PatchFavourite to receive a partial update body, with description and data as optional parameters (instead of adding each argument separately to the function). This would be represented as a struct with a "Description *string" and "Data *AssetData" fields.

As a minor point, if both Description and Data are nil (as in no changes were provided), the endpoint would return a 400 BadRequest status. If either/both are provided, assuming no other errors occurred, then the endpoint would return 200 Ok/204 NoContent, regardless of whether actual changes occurred or not. Although not providing data and providing unchanged data would be syntactically correct and have the same end result, semantically the 1st case would have no changes to apply, therefore no successful update can possibly occur.

mr-polymath added 3 commits October 20, 2025 16:55

Create favourites api with add, remove, update and edit endpoints. Ad…

4c4b1b3

…d seed data. Add basic authorization mechanism. Add basic tests.

Add swagger. Change AddFavourite to receive a reference of model.Asset

dc60e92

Add handler tests for PATCH and DELETE.

df64013

pamanta reviewed Oct 31, 2025

View reviewed changes

lkslts64 reviewed Oct 31, 2025

View reviewed changes

		@@ -0,0 +1,21 @@
		# Use the official Golang image
		FROM golang:latest

Conversation

mr-polymath commented Oct 20, 2025

Uh oh!

pamanta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkslts64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants