Skip to content

Submission: GWI Platform Go Challenge (Konstantinos Dasios)#52

Open
KostasDasios wants to merge 1 commit into
GlobalWebIndex:mainfrom
KostasDasios:main
Open

Submission: GWI Platform Go Challenge (Konstantinos Dasios)#52
KostasDasios wants to merge 1 commit into
GlobalWebIndex:mainfrom
KostasDasios:main

Conversation

@KostasDasios

Copy link
Copy Markdown

Dear GWI Team,

This pull request contains my completed submission for the GWI Platform Go Challenge.

Kind regards,
Konstantinos Dasios

@pamanta pamanta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @KostasDasios,

Thanks you very much for taking the time to complete the assignment.
Your submission was complete and very easy to follow!

I've left some comments for you to take a look when you have the time

Comment thread internal/server/server.go
Comment on lines +169 to +177
start := offset
if start > len(list) {
start = len(list)
}
end := start + limit
if end > len(list) {
end = len(list)
}
page := list[start:end]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the purpose of the assignment this is ok, but can you identify any potential drawbacks that might arise by doing the pagination here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main drawback is that we always load the entire favourites list into memory before slicing it.
For users with tens or hundreds of thousands of favourites, each request would unnecessarily allocate and iterate through the full dataset just to return a small window. This is wasteful in terms of both CPU and memory.

In a production environment I would push pagination down to the repository level, so the storage layer (e.g. PostgreSQL or MySQL) returns only the required page via an indexed query. This avoids loading the full dataset and keeps the API fast and efficient even at large scale.

Comment thread internal/server/server.go
Comment on lines +92 to +101
parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/")
if len(parts) < 3 || parts[0] != "users" || parts[2] != "favourites" {
http.NotFound(w, r)
return
}
userID := parts[1]
var favID string
if len(parts) >= 4 {
favID = parts[3]
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you identify any insufficiencies with this URL parsing mechanism? Specifically are there any edge case URLs accepted that are not part of the API contract?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, doing the URL parsing manually can let some edge cases slip through.
For example, paths like /users//favourites or /users/kostas/favourites/123/extra may still be accepted even though they shouldn't.

In a production service, I would avoid manual splitting and use a proper router such as Chi or Gin, which strictly enforce route patterns, handle path parameters safely, and reject invalid URLs by default.

Comment thread cmd/api/main.go
Comment on lines +46 to +48
// Gracefully shut down server with timeout
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

Comment thread internal/models/models.go
Comment on lines +50 to +56
type Favourite struct {
ID string `json:"id"`
Type AssetType `json:"type"`
Description string `json:"description,omitempty"`
Asset json.RawMessage `json:"asset"`
CreatedAt time.Time `json:"created_at"`
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you model this into a DB implementation? What type of DB would you use and why?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a relational database like PostgreSQL because this problem fits naturally with a relational model. We have clear entities (User, Asset, Favourite) and simple relationships between them. We also need consistency, constraints, and fast queries such as “get all favourites for this user,” which relational databases handle very well with indexing.

I would store favourites in a table like this:


CREATE TABLE favourites (
  id          UUID PRIMARY KEY,
  user_id     UUID NOT NULL,
  asset_type  TEXT NOT NULL,
  asset_id    UUID NOT NULL,
  description TEXT,
  created_at  TIMESTAMPTZ NOT NULL DEFAULT now(),
  UNIQUE (user_id, asset_type, asset_id)
);

CREATE INDEX idx_favourites_user_created_at
  ON favourites (user_id, created_at DESC);

The idea is simple: favourites links a user to a specific asset. Then each asset type (chart, insight, audience) can have its own table with its own structure. This keeps the design clean and makes it easy to query and paginate favourites efficiently.

return b
}

func TestService_CreateListUpdateDelete(t *testing.T) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is using mutexes for thread safety. How would you verify there are no race conditions? What tools would you use, and how would you re-structure the tests to protect against concurrency issues?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The in-memory store is protected with a sync.RWMutex, but I would still verify the concurrency behaviour explicitly to make sure there are no hidden data races.

Tools I would use

I would run the tests with Go’s race detector:

go test ./... -race

How I would test concurrency

I would add a dedicated test where many goroutines call Create, List and Delete at the same time, all using the same service instance.
Each goroutine would send any error into a channel (instead of calling t.Fatal inside the goroutine), and after all goroutines finish I would fail the test if any errors were reported.

Running this test multiple times helps flush out rare issues:

go test -race -count=50

How I would structure the tests

  • Each test should create its own fresh Service/repository instance so there is no shared global state between tests.
  • Tests that do not share any data can safely use t.Parallel().
  • Any shared data inside a test should follow the same locking rules as the service itself.

Comment thread Dockerfile

RUN go mod tidy || true

CMD ["/bin/bash"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain if this Dockerfile is functional? What was your goal using this command? Could you suggest any modifications to this file to make it usable?

@KostasDasios KostasDasios Nov 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last two lines in the Dockerfile were added only for convenience during local development:

RUN go mod tidy || true
CMD ["/bin/bash"]

During development I was entering the container with:

docker compose exec gwi-api bash

and running the service manually using:

go run ./cmd/api

This worked fine while iterating locally, but it’s not how a production-style Docker image should run.

  • go mod tidy || true prevented the build from failing the first time before modules were downloaded.
  • CMD ["/bin/bash"] allowed me to enter the container interactively while working locally (docker compose exec gwi-api bash).

These choices, however, are not correct for a final Docker image because:

  • they do not produce a runnable server,
  • || true hides real dependency issues,
  • having a shell as the entrypoint is unsuitable for production.

A cleaner approach is to let the Dockerfile focus strictly on building the Go binary, and let docker-compose handle development-specific behaviour

A more appropriate multi-stage Dockerfile for this project would look like:

FROM golang:1.25 AS builder
WORKDIR /app
COPY go.mod go.sum ./
RUN go mod download
COPY . .
RUN go build -o server ./cmd/api

FROM gcr.io/distroless/base
WORKDIR /
COPY --from=builder /app/server /server
EXPOSE 8080
CMD ["/server"]

This builds the Go server in a dedicated builder stage and runs only the final compiled binary in a minimal distroless image.

Comment thread README.md

### Option B — Docker
```bash
docker compose up --build -d

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please check if there are any missing steps in your docker instructions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Docker section could use a small clarification.
To run the service with Docker, the .env file needs to be present (either created manually or copied from .env.example), for example:

cp .env.example .env

This step should happen before running:

docker compose up --build -d

I would also add a short note that Docker and Docker Compose need to be installed, so the instructions are complete for someone setting up the project for the first time.

Comment thread docker-compose.yml
- "8080:8080"
env_file:
- .env
command: tail -f /dev/null

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate about the purpose of this command?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tail -f /dev/null line was used only during local development. It kept the container running so I could open an interactive shell using:

docker compose exec gwi-api bash

Inside that shell I was running the service with:

go run ./cmd/api

This works fine for development (and it’s also how I suggest running the app in the README), but it’s not how a production-style setup should behave. In a production image we typically want the container to run the compiled Go binary directly.

For example:

command: ["./server"]

or we can simply rely on the Dockerfile’s CMD instruction to start the server.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain the necessity of CORS handling considering the scope of the assignment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CORS handling is not strictly required for the core backend logic, but I added it for one practical reason during the assignment:
to allow the Swagger UI container (running on port 8081) to communicate with the API container (running on port 8080).

Since the two services run on different origins inside Docker Compose, the browser would block the requests without the proper CORS headers.
Without this middleware, Swagger UI would simply show “Failed to fetch” even though the API was working correctly.

I kept the middleware minimal and scoped only to local development. In a real deployment, the rules would be more restrictive or configured differently.

}

// NewRateLimiter constructs a new limiter enforcing one request every minInterval.
func NewRateLimiter(minInterval time.Duration) *RateLimiter {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain your decision of including the initialization as well as the implementation of the rate limiter in the middleware file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right that in a production codebase the rate limiter would normally live in its own small package (for example internal/ratelimit), with the middleware layer only wrapping it.

For the scope of this assignment I kept both the initialization and the limiter’s logic inside the middleware package to avoid introducing extra folders and keep the structure easy to follow during review. The goal was simply to show how the limiter plugs into the HTTP stack, rather than over-engineering the module layout.

In a real system I would extract it into its own package to keep responsibilities clearly separated, but for this challenge keeping it together made the overall solution simpler to navigate.

Comment thread internal/models/models.go
)

// AssetBase holds fields common to all assets.
type AssetBase struct {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you think of a different way of implementing this via using a different type?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used AssetBase as a small shared struct to keep the common fields in one place. It made the asset models simple and avoided repeating the same fields across ChartAsset, InsightAsset, and AudienceAsset.

Another valid option would be to rely fully on the AssetPayload interface and let each asset type define its own structure while still conforming to the interface.
This is in line with the point I mention in the next comment about type safety and keeping asset types clearly separated.

Comment thread internal/models/models.go
ID string `json:"id"`
Type AssetType `json:"type"`
Description string `json:"description,omitempty"`
Asset json.RawMessage `json:"asset"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please suggest an alternative approach of modeling your data overall to achieve type safety?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now Asset is a json.RawMessage, which is flexible but not type safe. A clearer approach is to treat the supported asset types as a fixed set (chart, insight, audience) and model each one with its own Go struct.

type AssetType string

const (
  AssetChart    AssetType = "chart"
  AssetInsight  AssetType = "insight"
  AssetAudience AssetType = "audience"
)

type ChartAsset struct { /* chart fields */ }
type InsightAsset struct { /* insight fields */ }
type AudienceAsset struct { /* audience fields */ }

type AssetPayload interface {
  AssetKind() AssetType
}

type Favourite struct {
  ID          string       `json:"id"`
  Type        AssetType    `json:"type"`
  Description string       `json:"description,omitempty"`
  Asset       AssetPayload `json:"asset"`
  CreatedAt   time.Time    `json:"created_at"`
}

Each asset type implements the AssetPayload interface, so the code can treat them all in a consistent way. This gives us a common contract (AssetKind()) while keeping each asset strongly typed.

We can also add small custom JSON marshal/unmarshal methods, so when the API receives a favourite, it automatically picks the correct struct based on the Type field.

Overall, this keeps the design simple, more type-safe, and easier to maintain as the number of asset types grows.

Comment thread internal/server/server.go

// NewServer builds a Server with an in-memory repository.
// Swap NewInMemoryRepo with a persistent implementation without touching handlers.
func NewServer(cfg *config.Config) *Server {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please discuss about the testability of the newServer method? Could you think of a different way of implementing this functionality?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more testable design would be to pass the dependencies in from the outside instead of creating them inside the function. For example:

func NewServer(cfg *config.Config, repo Repo, svc Service) *Server {
// wire router, middleware and handlers using repo and svc
}

This allows tests to provide in-memory or mock implementations, while production code can pass the real ones. That makes NewServer easier to test.

Comment thread internal/server/server.go

// Construct a lightweight rate limiter middleware based on environment config.
// Default: ~20 requests/sec per user or IP (configurable via RATE_LIMIT_MS).
rl := middleware.NewRateLimiter(time.Duration(cfg.RateLimitMillis) * time.Millisecond)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain how this rate limiter will work on a production environment where potentially more than one server instances will exist?
Could you suggest an alternative for enforcing rate limits on a production env?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rate limiter I added is an in-memory, in-process limiter. It works as expected in a single-instance or local development environment, but in a production setup with multiple server instances behind a load balancer each instance would keep its own counters. This means it would still protect each process from overload, but it would not enforce a strict global limit across all instances.

In a real production environment I would move rate limiting to a shared or edge layer. Two practical options are:

1. At the edge (reverse proxy / ingress)
Using nginx or Traefik to enforce limits per IP or per route before the requests reach the application. Both support consistent, cluster-wide rate limiting and are commonly used in production setups.

2. Distributed application-level rate limiting
Using a shared store such as Redis, so all instances check and update the same counters. This ensures limits are applied consistently across the entire cluster.

For the scope of this assignment I kept the simple in-memory version to demonstrate how such middleware plugs into the HTTP stack, but in production I would rely on an edge-level or distributed approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants