working IP address implementation and unit tests around GeoIP/full handler flow

src/http/Server.zig

@@ -1,5 +1,6 @@
 const std = @import("std");
 const httpz = @import("httpz");
+const build_options = @import("build_options");
 const handler = @import("handler.zig");
 const RateLimiter = @import("RateLimiter.zig");
 
@@ -91,3 +92,166 @@ pub fn listen(self: *Server) !void {
 pub fn deinit(self: *Server) void {
     self.httpz_server.deinit();
 }
+
+const MockHarness = struct {
+    allocator: std.mem.Allocator,
+    config: Config,
+    geoip: *GeoIp,
+    geocache: *GeoCache,
+    resolver: *Resolver,
+    mock: *Mock,
+    cache: *Cache,
+    opts: handler.HandleWeatherOptions,
+
+    const Mock = @import("../weather/Mock.zig");
+    const GeoCache = @import("../location/GeoCache.zig");
+    const GeoIp = @import("../location/GeoIp.zig");
+    const Resolver = @import("../location/resolver.zig").Resolver;
+    const Config = @import("../Config.zig");
+    const Cache = @import("../cache/Cache.zig");
+
+    pub fn init(allocator: std.mem.Allocator) !MockHarness {
+        const config = try Config.load(allocator);
+        errdefer config.deinit(allocator);
+
+        if (build_options.download_geoip) {
+            const GeoLite2 = @import("../location/GeoLite2.zig");
+            try GeoLite2.ensureDatabase(allocator, config.geolite_path);
+        }
+
+        const geoip = try allocator.create(GeoIp);
+        errdefer allocator.destroy(geoip);
+        geoip.* = GeoIp.init(allocator, config.geolite_path, null, null) catch {
+            allocator.destroy(geoip);
+            return error.SkipZigTest;
+        };
+        errdefer geoip.deinit();
+
+        var geocache = try allocator.create(GeoCache);
+        errdefer allocator.destroy(geocache);
+        geocache.* = try GeoCache.init(allocator, null);
+        errdefer geocache.deinit();
+
+        const resolver = try allocator.create(Resolver);
+        errdefer allocator.destroy(resolver);
+        resolver.* = Resolver.init(allocator, geoip, geocache, null);
+
+        const mock = try allocator.create(Mock);
+        errdefer allocator.destroy(mock);
+        mock.* = try Mock.init(allocator);
+        errdefer mock.deinit();
+
+        // Set a simple parse function that returns minimal weather data
+        mock.parse_fn = struct {
+            fn parse(_: *anyopaque, alloc: std.mem.Allocator, _: []const u8) anyerror!@import("../weather/types.zig").WeatherData {
+                const types = @import("../weather/types.zig");
+                return types.WeatherData{
+                    .allocator = alloc,
+                    .location = try alloc.dupe(u8, "Test"),
+                    .display_name = null,
+                    .coords = .{ .latitude = 0, .longitude = 0 },
+                    .current = .{
+                        .temp_c = 20,
+                        .feels_like_c = 20,
+                        .condition = try alloc.dupe(u8, "Clear"),
+                        .weather_code = .clear,
+                        .humidity = 50,
+                        .wind_kph = 5,
+                        .wind_deg = 0,
+                        .pressure_mb = 1013,
+                        .precip_mm = 0,
+                        .visibility_km = 10,
+                    },
+                    .forecast = &.{},
+                };
+            }
+        }.parse;
+
+        // Add wildcard response for tests
+        try mock.responses.put(try allocator.dupe(u8, "*"), try allocator.dupe(u8, "{}"));
+
+        var cache = try Cache.init(allocator, .{
+            .max_entries = 100,
+            .cache_dir = config.cache_dir,
+        });
+        errdefer cache.deinit();
+
+        return .{
+            .allocator = allocator,
+            .config = config,
+            .geoip = geoip,
+            .geocache = geocache,
+            .resolver = resolver,
+            .mock = mock,
+            .cache = cache,
+            .opts = .{
+                .provider = mock.provider(cache),
+                .resolver = resolver,
+                .geoip = geoip,
+            },
+        };
+    }
+
+    pub fn deinit(self: *MockHarness) void {
+        self.cache.deinit();
+        self.mock.deinit();
+        self.allocator.destroy(self.mock);
+        self.geocache.deinit();
+        self.allocator.destroy(self.geocache);
+        self.allocator.destroy(self.resolver);
+        self.geoip.deinit();
+        self.allocator.destroy(self.geoip);
+        self.config.deinit(self.allocator);
+    }
+};
+
+test "handleWeather: default endpoint uses IP address" {
+    const allocator = std.testing.allocator;
+
+    var harness = try MockHarness.init(allocator);
+    defer harness.deinit();
+
+    var ht = httpz.testing.init(.{});
+    defer ht.deinit();
+
+    ht.url("/");
+    ht.header("x-forwarded-for", "73.158.64.1");
+
+    try handler.handleWeather(&harness.opts, ht.req, ht.res);
+
+    try ht.expectStatus(200);
+}
+
+test "handleWeather: x-forwarded-for with multiple IPs" {
+    const allocator = std.testing.allocator;
+    var harness = try MockHarness.init(allocator);
+    defer harness.deinit();
+
+    var ht = httpz.testing.init(.{});
+    defer ht.deinit();
+
+    ht.url("/");
+    ht.header("x-forwarded-for", "73.158.64.1, 8.8.8.8, 192.168.1.1");
+
+    try handler.handleWeather(&harness.opts, ht.req, ht.res);
+
+    try ht.expectStatus(200);
+}
+test "handleWeather: client IP only" {
+    const allocator = std.testing.allocator;
+
+    var harness = try MockHarness.init(allocator);
+    defer harness.deinit();
+
+    var ht = httpz.testing.init(.{});
+    defer ht.deinit();
+
+    // Set connection address to a valid IP that will be in GeoIP database
+    ht.conn.address = try std.net.Address.parseIp("73.158.64.1", 0);
+
+    ht.url("/");
+
+    try handler.handleWeather(&harness.opts, ht.req, ht.res);
+
+    try ht.expectStatus(200);
+}

src/http/handler.zig

@@ -10,6 +10,8 @@ const v2 = @import("../render/v2.zig");
 const custom = @import("../render/custom.zig");
 const help = @import("help.zig");
 
+const log = std.log.scoped(.handler);
+
 pub const HandleWeatherOptions = struct {
     provider: WeatherProvider,
     resolver: *Resolver,
@@ -47,6 +49,7 @@ pub fn handleWeather(
         } else break :blk client_ip; // no location, just use client ip instead
     };
 
+    log.debug("location = {s}, client_ip = {s}", .{ location, client_ip });
     if (location.len == 0) {
         res.content_type = .TEXT;
         res.body = "Sorry, we are unable to determine your location at this time. Try with /<location> or /?location=<location>\n";
@@ -113,20 +116,22 @@ fn handleWeatherInternal(
     const location = opts.resolver.resolve(location_query) catch |err| {
         switch (err) {
             error.LocationNotFound => {
+                log.debug("Location not found for query {s}", .{location_query});
                 res.status = 404;
-                res.body = "Location not found\n";
+                res.body = "Location not found (location query resolution failure)\n";
                 return;
             },
             else => return err,
         }
     };
+    defer location.deinit();
 
     // Fetch weather using coordinates
     var weather = opts.provider.fetch(req_alloc, location.coords) catch |err| {
         switch (err) {
             error.LocationNotFound => {
                 res.status = 404;
-                res.body = "Location not found\n";
+                res.body = "Location not found (provider fetch failure)\n";
                 return;
             },
             else => return err,
@@ -147,14 +152,13 @@ fn handleWeatherInternal(
     // Determine if imperial units should be used
     // Priority: explicit ?u or ?m > lang=us > US IP > default metric
     const use_imperial = blk: {
-        if (params.units) |u| {
+        if (params.units) |u|
             break :blk u == .uscs;
-        }
-        if (params.lang) |lang| {
-            if (std.mem.eql(u8, lang, "us")) {
+
+        if (params.lang) |lang|
+            if (std.mem.eql(u8, lang, "us"))
                 break :blk true;
-            }
-        }
+
         if (client_ip.len > 0 and opts.geoip.isUSIp(client_ip))
             break :blk true;
         break :blk false;
@@ -164,13 +168,13 @@ fn handleWeatherInternal(
         if (std.mem.eql(u8, fmt, "j1")) {
             res.content_type = .JSON;
             break :blk try json.render(req_alloc, weather);
-        } else if (std.mem.eql(u8, fmt, "v2")) {
-            break :blk try v2.render(req_alloc, weather, use_imperial);
-        } else if (std.mem.startsWith(u8, fmt, "%")) {
-            break :blk try custom.render(req_alloc, weather, fmt, use_imperial);
-        } else {
-            break :blk try line.render(req_alloc, weather, fmt, use_imperial);
         }
+        if (std.mem.eql(u8, fmt, "v2"))
+            break :blk try v2.render(req_alloc, weather, use_imperial);
+        if (std.mem.startsWith(u8, fmt, "%"))
+            break :blk try custom.render(req_alloc, weather, fmt, use_imperial);
+        // fall back to line if we don't understant the format parameter
+        break :blk try line.render(req_alloc, weather, fmt, use_imperial);
     } else try formatted.render(req_alloc, weather, .{ .use_imperial = use_imperial });
 
     // Add coordinates header using response allocator

src/location/GeoIp.zig

@@ -7,8 +7,9 @@ const c = @cImport({
 });
 
 const GeoIP = @This();
+const log = std.log.scoped(.geoip);
 
-mmdb: c.MMDB_s,
+mmdb: *c.MMDB_s,
 ip2location_client: ?*Ip2location,
 ip2location_cache: ?*Ip2location.Cache,
 allocator: std.mem.Allocator,
@@ -17,11 +18,13 @@ pub fn init(allocator: std.mem.Allocator, db_path: []const u8, api_key: ?[]const
     const path_z = try std.heap.c_allocator.dupeZ(u8, db_path);
     defer std.heap.c_allocator.free(path_z);
 
-    // SAFETY: The C API will initialize this on the next line
-    var mmdb: c.MMDB_s = undefined;
-    const status = c.MMDB_open(path_z.ptr, c.MMDB_MODE_MMAP, &mmdb);
-    if (status != c.MMDB_SUCCESS)
+    const mmdb = try allocator.create(c.MMDB_s);
+    errdefer allocator.destroy(mmdb);
+
+    const status = c.MMDB_open(path_z.ptr, c.MMDB_MODE_MMAP, mmdb);
+    if (status != c.MMDB_SUCCESS) {
         return error.CannotOpenDatabase;
+    }
 
     var client: ?*Ip2location = null;
     var cache: ?*Ip2location.Cache = null;
@@ -50,7 +53,8 @@ pub fn init(allocator: std.mem.Allocator, db_path: []const u8, api_key: ?[]const
 }
 
 pub fn deinit(self: *GeoIP) void {
-    c.MMDB_close(&self.mmdb);
+    c.MMDB_close(self.mmdb);
+    self.allocator.destroy(self.mmdb);
     if (self.ip2location_client) |client| {
         client.deinit();
         self.allocator.destroy(client);
@@ -61,18 +65,18 @@ pub fn deinit(self: *GeoIP) void {
     }
 }
 
-pub fn lookup(self: *GeoIP, ip: []const u8) !?Coordinates {
+pub fn lookup(self: *GeoIP, ip: []const u8) ?Coordinates {
     // Try MaxMind first
-    const result = lookupInternal(&self.mmdb, ip) catch return null;
+    const result = lookupInternal(self.mmdb, ip) catch return null;
 
-    if (result.found_entry) {
-        return try self.extractCoordinates(result);
-    }
+    log.debug("lookup geoip db for ip {s}. Found: {}", .{ ip, result.found_entry });
+    if (result.found_entry)
+        if (self.extractCoordinates(ip, result)) |coords|
+            return coords;
 
     // Fallback to IP2Location if configured
-    if (self.ip2location_client) |client| {
+    if (self.ip2location_client) |client|
         return client.lookupWithCache(ip, self.ip2location_cache);
-    }
 
     return null;
 }
@@ -85,45 +89,69 @@ fn lookupInternal(mmdb: *c.MMDB_s, ip: []const u8) !c.MMDB_lookup_result_s {
     var mmdb_error: c_int = 0;
 
     const result = c.MMDB_lookup_string(mmdb, ip_z.ptr, &gai_error, &mmdb_error);
-    if (gai_error != 0 or mmdb_error != 0) return error.MMDBLookupError;
+    if (mmdb_error != 0) {
+        log.warn("got error on MMDB_lookup_string for ip {s}. gai = {d}, mmdb_error = {d}", .{ ip, gai_error, mmdb_error });
+        return error.MMDBLookupError;
+    }
     return result;
 }
 
 pub fn isUSIp(self: *GeoIP, ip: []const u8) bool {
-    const result = lookupInternal(&self.mmdb, ip) catch return false;
+    var result = lookupInternal(self.mmdb, ip) catch return false;
     if (!result.found_entry) return false;
 
-    var entry_mut = result.entry;
-    const null_term: [*:0]const u8 = @ptrCast(&[_]u8{0});
     // SAFETY: The C API will initialize this on the next line
     var country_data: c.MMDB_entry_data_s = undefined;
-    const status = c.MMDB_get_value(&entry_mut, &country_data, "country\x00", "iso_code\x00", null_term);
+    const status = c.MMDB_get_value(&result.entry, &country_data, "country", "iso_code", @as([*c]const u8, null));
 
-    if (status != c.MMDB_SUCCESS or !country_data.has_data)
+    if (status != c.MMDB_SUCCESS or !country_data.has_data) {
+        log.info("lookup found result, but no country available in data for ip {s}. MMDB_get_value returned {d}", .{ ip, status });
         return false;
+    }
 
-    const country_code = std.mem.span(country_data.unnamed_0.utf8_string);
+    const country_code = country_data.unnamed_0.utf8_string[0..country_data.data_size];
     return std.mem.eql(u8, country_code, "US");
 }
 
-fn extractCoordinates(self: *GeoIP, result: c.MMDB_lookup_result_s) !Coordinates {
+fn extractCoordinates(self: *GeoIP, ip: []const u8, result: c.MMDB_lookup_result_s) ?Coordinates {
     _ = self;
-    var entry_mut = result.entry;
 
-    // SAFETY: The C API will initialize below
+    if (!result.found_entry) return null;
+
+    var entry_copy = result.entry;
+
+    // SAFETY: latitude_data set by MMDB_get_value
     var latitude_data: c.MMDB_entry_data_s = undefined;
-    // SAFETY: The C API will initialize below
+    const lat_status = c.MMDB_get_value(&entry_copy, &latitude_data, "location", "latitude", @as([*c]const u8, null));
+
+    if (lat_status != c.MMDB_SUCCESS or !latitude_data.has_data) {
+        log.info("lookup found result, but no latitude available in data for ip {s}. MMDB_get_value returned {d}", .{ ip, lat_status });
+        return null;
+    }
+
+    // SAFETY: longitude_data set by MMDB_get_value
     var longitude_data: c.MMDB_entry_data_s = undefined;
+    const lon_status = c.MMDB_get_value(&entry_copy, &longitude_data, "location", "longitude", @as([*c]const u8, null));
 
-    const lat_status = c.MMDB_get_value(&entry_mut, &latitude_data, "location", "latitude", @as([*:0]const u8, @ptrCast(&[_]u8{0})));
-    const lon_status = c.MMDB_get_value(&entry_mut, &longitude_data, "location", "longitude", @as([*:0]const u8, @ptrCast(&[_]u8{0})));
+    if (lon_status != c.MMDB_SUCCESS or !longitude_data.has_data) {
+        log.info("lookup found result, but no longitude available in data for ip {s}. MMDB_get_value returned {d}", .{ ip, lon_status });
+        return null;
+    }
 
-    if (lat_status != c.MMDB_SUCCESS or lon_status != c.MMDB_SUCCESS or !latitude_data.has_data or !longitude_data.has_data)
-        return error.CoordinatesNotFound;
+    var coords = [_]f64{ latitude_data.unnamed_0.double_value, longitude_data.unnamed_0.double_value };
+
+    // Depending on how this is compiled, the byteswap may or may not be necessary
+    // original c, compiled with zig, statically linked: byteSwap
+    // pre=built, dynamically linked, do not byte swap
+    // I'm not sure precisely what causes this
+    std.mem.byteSwapAllElements(f64, &coords);
+
+    const latitude = coords[0];
+    const longitude = coords[1];
 
     return .{
-        .latitude = latitude_data.unnamed_0.double_value,
-        .longitude = longitude_data.unnamed_0.double_value,
+        .latitude = latitude,
+        .longitude = longitude,
     };
 }
 
@@ -156,10 +184,35 @@ test "isUSIP detects US IPs" {
     defer geoip.deinit();
 
     // Test that the function doesn't crash with various IPs
-    _ = geoip.isUSIp("8.8.8.8");
-    _ = geoip.isUSIp("1.1.1.1");
+    try std.testing.expect(geoip.isUSIp("73.158.64.1"));
 
     // Test invalid IP returns false
     const invalid = geoip.isUSIp("invalid");
     try std.testing.expect(!invalid);
 }
+test "lookup works" {
+    const allocator = std.testing.allocator;
+    const Config = @import("../Config.zig");
+    const config = try Config.load(allocator);
+    defer config.deinit(allocator);
+    const build_options = @import("build_options");
+    const db_path = config.geolite_path;
+
+    if (build_options.download_geoip) {
+        const GeoLite2 = @import("GeoLite2.zig");
+        try GeoLite2.ensureDatabase(std.testing.allocator, db_path);
+    }
+
+    var geoip = GeoIP.init(std.testing.allocator, db_path, null, null) catch {
+        return error.SkipZigTest;
+    };
+    defer geoip.deinit();
+
+    // Test that the function doesn't crash with various IPs
+    const maybe_coords = geoip.lookup("73.158.64.1");
+
+    try std.testing.expect(maybe_coords != null);
+
+    const coords = maybe_coords.?;
+    try std.testing.expectEqual(@as(f64, 37.5958), coords.latitude);
+}

src/location/Ip2location.zig

@@ -23,7 +23,7 @@ pub fn deinit(self: *Self) void {
     self.allocator.free(self.api_key);
 }
 
-pub fn lookupWithCache(self: *Self, ip_str: []const u8, cache: ?*Cache) !?Coordinates {
+pub fn lookupWithCache(self: *Self, ip_str: []const u8, cache: ?*Cache) ?Coordinates {
     // Parse IP to u128 for cache lookup
     const addr = std.net.Address.parseIp(ip_str, 0) catch return null;
     const ip_u128: u128 = switch (addr.any.family) {
@@ -59,6 +59,7 @@ pub fn lookupWithCache(self: *Self, ip_str: []const u8, cache: ?*Cache) !?Coordi
 pub fn lookup(self: *Self, ip_str: []const u8) !Coordinates {
     log.info("Fetching geolocation for IP {s}", .{ip_str});
 
+    if (@import("builtin").is_test) return error.LookupUnavailableInUnitTest;
     // Build URL: https://api.ip2location.io/?key=XXX&ip=1.2.3.4
     const url = try std.fmt.allocPrint(
         self.allocator,

src/location/resolver.zig

@@ -9,6 +9,11 @@ const log = std.log.scoped(.resolver);
 pub const Location = struct {
     name: []const u8,
     coords: Coordinates,
+    allocator: std.mem.Allocator,
+
+    pub fn deinit(self: Location) void {
+        self.allocator.free(self.name);
+    }
 };
 
 pub const LocationType = enum {
@@ -37,6 +42,7 @@ pub const Resolver = struct {
     pub fn resolve(self: *Resolver, query: []const u8) !Location {
         const location_type = detectType(query);
 
+        log.debug("location type: {}", .{location_type});
         return switch (location_type) {
             .ip_address => try self.resolveIP(query),
             .domain_name => try self.resolveDomain(query[1..]), // Skip '@'
@@ -57,8 +63,9 @@ pub const Resolver = struct {
 
     fn resolveIP(self: *Resolver, ip: []const u8) !Location {
         if (self.geoip) |geoip| {
-            if (try geoip.lookup(ip)) |coords| {
+            if (geoip.lookup(ip)) |coords| {
                 return .{
+                    .allocator = self.allocator,
                     .name = try self.allocator.dupe(u8, ip),
                     .coords = coords,
                 };
@@ -91,12 +98,14 @@ pub const Resolver = struct {
         // Check cache first
         if (self.geocache.get(name)) |cached| {
             return Location{
+                .allocator = self.allocator,
                 .name = try self.allocator.dupe(u8, cached.name),
                 .coords = cached.coords,
             };
         }
 
         log.info("Calling nominatim (OpenStreetMap) to resolve place name {s} to coordinates", .{name});
+        if (@import("builtin").is_test) return error.GeocodingUnavailableInUnitTest;
         // Call Nominatim API
         const url = try std.fmt.allocPrint(
             self.allocator,
@@ -158,7 +167,8 @@ pub const Resolver = struct {
             },
         });
 
-        return Location{
+        return .{
+            .allocator = self.allocator,
             .name = try self.allocator.dupe(u8, display_name),
             .coords = .{
                 .latitude = lat,
@@ -178,6 +188,7 @@ pub const Resolver = struct {
 
             if (airports.lookup(&upper_code)) |airport| {
                 return Location{
+                    .allocator = self.allocator,
                     .name = try self.allocator.dupe(u8, airport.name),
                     .coords = airport.coords,
                 };
@@ -230,3 +241,35 @@ test "resolver init" {
     try std.testing.expect(resolver.geoip == null);
     try std.testing.expect(resolver.airports == null);
 }
+
+test "resolve IP address with GeoIP" {
+    const allocator = std.testing.allocator;
+    const Config = @import("../Config.zig");
+    const config = try Config.load(allocator);
+    defer config.deinit(allocator);
+
+    const build_options = @import("build_options");
+    if (build_options.download_geoip) {
+        const GeoLite2 = @import("GeoLite2.zig");
+        try GeoLite2.ensureDatabase(allocator, config.geolite_path);
+    }
+
+    var geoip = GeoIp.init(allocator, config.geolite_path, null, null) catch {
+        return error.SkipZigTest;
+    };
+    defer geoip.deinit();
+
+    var geocache = try GeoCache.init(allocator, null);
+    defer geocache.deinit();
+
+    var resolver = Resolver.init(allocator, &geoip, &geocache, null);
+
+    // Use IP that's known to have coordinates in GeoLite2 database
+    const test_ip = "73.158.64.1";
+
+    const location = try resolver.resolve(test_ip);
+    defer location.deinit();
+
+    try std.testing.expectEqualStrings(test_ip, location.name);
+    try std.testing.expect(location.coords.latitude != 0 or location.coords.longitude != 0);
+}

src/weather/Mock.zig

@@ -40,7 +40,8 @@ fn fetchRaw(ptr: *anyopaque, allocator: std.mem.Allocator, coords: Coordinates)
     const key = try std.fmt.allocPrint(allocator, "{d:.4},{d:.4}", .{ coords.latitude, coords.longitude });
     defer allocator.free(key);
 
-    const raw = self.responses.get(key) orelse return error.LocationNotFound;
+    // Try exact match first, then wildcard
+    const raw = self.responses.get(key) orelse self.responses.get("*") orelse return error.LocationNotFound;
     return try allocator.dupe(u8, raw);
 }