feat(geo): add IP geolocation via ipapi.co - #32
Merged
Merged
Conversation
Integrates ipapi.co to resolve requester IPs to geographic metadata. The client is always initialised (free tier works without a key); Redis caching (24 h TTL) is used when REDIS_URL is set. - domain/geolocation.go: GeoLocation entity (IP, CountryCode, CountryName, Region, City, Timezone, Currency, IsEU) - usecase/geolocation.go: GeoLocator interface - infrastructure/ipgeo/ipapi_client.go: full rewrite — New/NewWithBaseURL constructors, private-IP short-circuit, cache-aside lookup, optional API key (?key= param), User-Agent: template-backend/1.0 - transport/middleware/geo.go: GeoFromRequest best-effort middleware + exported RealIP helper (XFF → X-Real-IP → RemoteAddr precedence) - Bootstrap: GeoLocator always wired in Run(); IPAPIKey added to Config - Handler: geoLocator field (10th param); applied to /api/v1 group - .env.example: IPAPI_KEY= (optional, free tier comment) - Docs: geo.md created; environment.md, bootstrap.md, middleware.md updated Closes #23
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds end-to-end IP geolocation support via ipapi.co. Introduces ChangesIP Geolocation Feature
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant GeoFromRequest
participant ipgeo_Client as ipgeo.Client
participant Redis
participant ipapi_co as ipapi.co
Client->>GeoFromRequest: HTTP request to /api/v1/...
GeoFromRequest->>GeoFromRequest: RealIP (X-Forwarded-For / X-Real-IP / RemoteAddr)
GeoFromRequest->>ipgeo_Client: Lookup(ctx, ip)
ipgeo_Client->>ipgeo_Client: isPrivateIP? → ErrPrivateIP (skip cache/HTTP)
ipgeo_Client->>Redis: Get("geo:<ip>")
alt cache hit
Redis-->>ipgeo_Client: cached GeoLocation JSON
ipgeo_Client-->>GeoFromRequest: *domain.GeoLocation
else cache miss
Redis-->>ipgeo_Client: miss
ipgeo_Client->>ipapi_co: GET /<ip>/json[?key=IPAPI_KEY]
ipapi_co-->>ipgeo_Client: JSON (country/region/city/...)
ipgeo_Client->>Redis: Set("geo:<ip>", json, 24h) [best-effort]
ipgeo_Client-->>GeoFromRequest: *domain.GeoLocation
end
GeoFromRequest->>GeoFromRequest: c.Set(GeoLocationKey, geo)
GeoFromRequest->>Client: c.Next() → handler response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/internal/infrastructure/ipgeo/ipapi_client_test.go (2)
98-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a malformed-IP case that asserts no outbound HTTP call.
Please add a test for an invalid IP (for example
"not-an-ip") and verify lookup fails before any server call. This protects the no-network short-circuit behavior against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/infrastructure/ipgeo/ipapi_client_test.go` around lines 98 - 121, Add a test case for malformed IP addresses (such as "not-an-ip") within the TestClient_Lookup_PrivateIP function or create a separate test function TestClient_Lookup_MalformedIP. Similar to the private IP test, verify that when client.Lookup is called with an invalid IP string, it returns an appropriate error before making any HTTP requests, and confirm that the HTTP server callback (the called flag) remains false to ensure the lookup short-circuits invalid input validation before attempting network calls.
30-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAssert cache TTL to lock the 24h contract.
mockCacheService.Setcurrently ignores the TTL argument, soTestClient_Lookup_CacheHitcan’t detect TTL regressions. Capture the duration and assert24*time.Hour.🔧 Suggested test improvement
type mockCacheService struct { store map[string]string + lastTTL time.Duration } @@ func (m *mockCacheService) Set(_ context.Context, key string, value string, _ time.Duration) error { m.store[key] = value + m.lastTTL = ttl return nil } @@ if callCount != 1 { t.Errorf("expected still 1 HTTP call after cache hit, got %d", callCount) } + if cache.lastTTL != 24*time.Hour { + t.Errorf("cache TTL: got %v, want %v", cache.lastTTL, 24*time.Hour) + }Also applies to: 155-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/infrastructure/ipgeo/ipapi_client_test.go` around lines 30 - 33, In the mockCacheService.Set method, the TTL duration parameter is currently ignored using the blank identifier. Modify the mock struct to add a field to store the duration argument, then update the Set method to capture the duration parameter into this field instead of ignoring it. Finally, add an assertion in TestClient_Lookup_CacheHit (and any other relevant tests) to verify that the captured TTL equals 24*time.Hour to ensure the cache TTL contract is maintained and detect potential regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/infrastructure/ipgeo/ipapi_client.go`:
- Around line 76-79: Add IP format validation at the beginning of the Lookup
method before the isPrivateIP check to reject malformed IP strings early and
prevent unnecessary external calls. Use Go's net package to validate the IP
format (e.g., net.ParseIP) and return an appropriate error if the IP is invalid,
ensuring that untrusted header input cannot trigger external API calls and
rate-limit pressure on the downstream service.
- Around line 83-84: In the `Lookup` method of the `ipapi_client.go` file,
handle cache read and write errors instead of silently ignoring them. When
calling `c.cache.Get` on line 83-84, if the error is not nil, return the wrapped
error to the caller instead of continuing execution. Similarly, for the cache
write operations around lines 99-101, capture and return any errors from cache
write calls instead of discarding them. This ensures infrastructure faults in
Redis are properly surfaced and prevents non-deterministic behavior under cache
failures.
In `@backend/internal/transport/middleware/geo.go`:
- Around line 34-38: The IP extraction logic in the function containing lines 34
and 37 unconditionally trusts the X-Forwarded-For and X-Real-IP headers without
validating that they come from a trusted proxy, allowing clients to forge these
headers and spoof their IP address. Modify the logic to only extract IPs from
these forwarding headers when the request originates from a trusted proxy or
reverse proxy. This typically means checking if the immediate upstream
connection is from a known trusted source before using the forwarding headers;
otherwise, fall back to extracting the IP directly from the request's remote
address to prevent IP spoofing attacks.
---
Nitpick comments:
In `@backend/internal/infrastructure/ipgeo/ipapi_client_test.go`:
- Around line 98-121: Add a test case for malformed IP addresses (such as
"not-an-ip") within the TestClient_Lookup_PrivateIP function or create a
separate test function TestClient_Lookup_MalformedIP. Similar to the private IP
test, verify that when client.Lookup is called with an invalid IP string, it
returns an appropriate error before making any HTTP requests, and confirm that
the HTTP server callback (the called flag) remains false to ensure the lookup
short-circuits invalid input validation before attempting network calls.
- Around line 30-33: In the mockCacheService.Set method, the TTL duration
parameter is currently ignored using the blank identifier. Modify the mock
struct to add a field to store the duration argument, then update the Set method
to capture the duration parameter into this field instead of ignoring it.
Finally, add an assertion in TestClient_Lookup_CacheHit (and any other relevant
tests) to verify that the captured TTL equals 24*time.Hour to ensure the cache
TTL contract is maintained and detect potential regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78595098-b9f3-4aa6-aa0d-b5560efb5470
📒 Files selected for processing (17)
backend/.env.examplebackend/docs/_index.mdbackend/docs/bootstrap.mdbackend/docs/environment.mdbackend/docs/geo.mdbackend/docs/middleware.mdbackend/internal/bootstrap/bootstrap.gobackend/internal/domain/geolocation.gobackend/internal/infrastructure/ipgeo/ipapi_client.gobackend/internal/infrastructure/ipgeo/ipapi_client_test.gobackend/internal/server/server.gobackend/internal/transport/handlers/handler.gobackend/internal/transport/handlers/health_handler_test.gobackend/internal/transport/handlers/routes.gobackend/internal/transport/middleware/geo.gobackend/internal/transport/middleware/geo_test.gobackend/internal/usecase/geolocation.go
- Reject malformed IP strings before isPrivateIP/fetch — net.ParseIP returning nil now returns an error immediately, preventing unnecessary outbound calls from untrusted header input - Log cache read/write errors via slog.WarnContext instead of silently swallowing them; errors are non-fatal (cache miss falls through to HTTP, cache write failure still returns valid geo data) - RealIP now only trusts X-Forwarded-For / X-Real-IP when RemoteAddr is a private or loopback address (i.e. the connection came through a trusted proxy); direct clients with public RemoteAddr cannot spoof the originating IP via forwarding headers - Add TestClient_Lookup_InvalidIP and TestRealIP_XForwardedFor_IgnoredFromPublicAddr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
REDIS_URLis setGeoFromRequestmiddleware attaches*domain.GeoLocationto the Gin context; best-effort — never aborts the request on failureipgeoimplementation that only returned lat/lon into the full featureChanges
New files
internal/domain/geolocation.go—GeoLocationentityinternal/usecase/geolocation.go—GeoLocatorinterfaceinternal/transport/middleware/geo.go—GeoFromRequestmiddleware +RealIPhelper (XFF → X-Real-IP → RemoteAddr)backend/docs/geo.md— full topic docUpdated files
infrastructure/ipgeo/ipapi_client.go— rewritten:New/NewWithBaseURLconstructors, private-IP short-circuit, cache-aside with 24 h TTL, optional?key=param,User-Agent: template-backend/1.0GeoLocatoralways wired inRun();IPAPIKeyinConfiggeoLocatoras 10th field;GeoFromRequestapplied to/api/v1groupbackend/.env.example:IPAPI_KEY=(optional)environment.md,bootstrap.md,middleware.mdupdatedTest plan
go test ./internal/infrastructure/ipgeo/...— 6 unit tests (valid IP, private IP, API error, non-200, cache hit, API key param)go test ./internal/transport/middleware/...— 6 tests (geo attach, skip on error, skip on private IP, RealIP variants)/api/v1route from a public IP and read thegeo_locationkey from contextIPAPI_KEYis unset (free tier)REDIS_URLis unset (no caching, still works)Closes #23
Summary by CodeRabbit
New Features
Documentation
IPAPI_KEYas an optional environment variable (free tier works without it).Tests