From 0b968c3b44759bd7662b48a79c3a13e89753afcf Mon Sep 17 00:00:00 2001 From: Bob Farrell Date: Sat, 13 Apr 2024 19:24:40 +0100 Subject: [PATCH] Minor improvements to request params and headers Fully disambiguate request params from body and query string and clean up convoluted code, reduce allocs. Implement `jetzig.http.Request.queryParams` which always returns only the parsed query string, never the request body. Deprecate `headers.getFirstValue` in favour of `headers.get`, implement `headers.getAll`. --- build.zig.zon | 4 +- src/GenerateRoutes.zig | 7 +-- src/jetzig/http/Headers.zig | 74 ++++++++++++++++++++------ src/jetzig/http/Query.zig | 22 ++++---- src/jetzig/http/Request.zig | 103 +++++++++++++++++++----------------- 5 files changed, 127 insertions(+), 83 deletions(-) diff --git a/build.zig.zon b/build.zig.zon index 9a8bcbc..c4730d2 100644 --- a/build.zig.zon +++ b/build.zig.zon @@ -7,8 +7,8 @@ .hash = "12207d49df326e0c180a90fa65d9993898e0a0ffd8e79616b4b81f81769261858856", }, .zmpl = .{ - .url = "https://github.com/jetzig-framework/zmpl/archive/09c04d91d18ec7fd82ac444b1e7e609380274098.tar.gz", - .hash = "12205356d83fa54a6e2c81cd5dc461fa0a1d042af379c7308aea0bf3fdb9d80a709c", + .url = "https://github.com/jetzig-framework/zmpl/archive/6f9496a02dcdc2a309b2c1bbaec2affc55f063ae.tar.gz", + .hash = "122014e63f5421060573216929af3a4c4c373934213ed727c77af927f787cb2f4113", }, .args = .{ .url = "https://github.com/MasterQ32/zig-args/archive/01d72b9a0128c474aeeb9019edd48605fa6d95f7.tar.gz", diff --git a/src/GenerateRoutes.zig b/src/GenerateRoutes.zig index c638798..389a628 100644 --- a/src/GenerateRoutes.zig +++ b/src/GenerateRoutes.zig @@ -253,14 +253,11 @@ fn generateRoutesForView(self: *Self, views_dir: std.fs.Dir, path: []const u8) ! if (static_params) |capture| { if (capture.get(static_route.name)) |params| { for (params.array.array.items) |item| { // XXX: Use public interface for Data.Array here ? - var json_buf = std.ArrayList(u8).init(self.allocator); - defer json_buf.deinit(); - const json_writer = json_buf.writer(); - try item.toJson(json_writer, false, 0); + const json = try item.toJson(); var encoded_buf = std.ArrayList(u8).init(self.allocator); defer encoded_buf.deinit(); const writer = encoded_buf.writer(); - try std.json.encodeJsonString(json_buf.items, .{}, writer); + try std.json.encodeJsonString(json, .{}, writer); try static_route.params.append(try self.allocator.dupe(u8, encoded_buf.items)); } } diff --git a/src/jetzig/http/Headers.zig b/src/jetzig/http/Headers.zig index b49027a..ec65a02 100644 --- a/src/jetzig/http/Headers.zig +++ b/src/jetzig/http/Headers.zig @@ -4,36 +4,55 @@ const jetzig = @import("../../jetzig.zig"); allocator: std.mem.Allocator, headers: HeadersArray, -const Self = @This(); +const Headers = @This(); pub const max_headers = 25; const HeadersArray = std.ArrayListUnmanaged(std.http.Header); -pub fn init(allocator: std.mem.Allocator) Self { +pub fn init(allocator: std.mem.Allocator) Headers { return .{ .allocator = allocator, .headers = HeadersArray.initCapacity(allocator, max_headers) catch @panic("OOM"), }; } -pub fn deinit(self: *Self) void { +pub fn deinit(self: *Headers) void { self.headers.deinit(self.allocator); } -// Gets the first value for a given header identified by `name`. Names are case insensitive. -pub fn getFirstValue(self: *Self, name: []const u8) ?[]const u8 { +/// Gets the first value for a given header identified by `name`. Names are case insensitive. +pub fn get(self: Headers, name: []const u8) ?[]const u8 { for (self.headers.items) |header| { if (jetzig.util.equalStringsCaseInsensitive(name, header.name)) return header.value; } return null; } +/// Gets the first value for a given header identified by `name`. Names are case insensitive. +pub fn getAll(self: Headers, name: []const u8) []const []const u8 { + var headers = std.ArrayList([]const u8).init(self.allocator); + + for (self.headers.items) |header| { + if (jetzig.util.equalStringsCaseInsensitive(name, header.name)) { + headers.append(header.value) catch @panic("OOM"); + } + } + return headers.toOwnedSlice() catch @panic("OOM"); +} + +// Deprecated +pub fn getFirstValue(self: *Headers, name: []const u8) ?[]const u8 { + return self.get(name); +} + /// Appends `name` and `value` to headers. -pub fn append(self: *Self, name: []const u8, value: []const u8) !void { +pub fn append(self: *Headers, name: []const u8, value: []const u8) !void { + if (self.headers.items.len >= 25) return error.JetzigTooManyHeaders; + self.headers.appendAssumeCapacity(.{ .name = name, .value = value }); } /// Removes **all** header entries matching `name`. Names are case-insensitive. -pub fn remove(self: *Self, name: []const u8) void { +pub fn remove(self: *Headers, name: []const u8) void { if (self.headers.items.len == 0) return; var index: usize = self.headers.items.len; @@ -47,13 +66,13 @@ pub fn remove(self: *Self, name: []const u8) void { } /// Returns an iterator which implements `next()` returning each name/value of the stored headers. -pub fn iterator(self: *Self) Iterator { +pub fn iterator(self: Headers) Iterator { return Iterator{ .headers = self.headers }; } /// Returns an array of `std.http.Header`, can be used to set response headers directly. /// Caller owns memory. -pub fn stdHeaders(self: *Self) !std.ArrayListUnmanaged(std.http.Header) { +pub fn stdHeaders(self: *Headers) !std.ArrayListUnmanaged(std.http.Header) { var array = try std.ArrayListUnmanaged(std.http.Header).initCapacity(self.allocator, max_headers); var it = self.iterator(); @@ -87,24 +106,45 @@ const Iterator = struct { test "append" { const allocator = std.testing.allocator; - var headers = Self.init(allocator); + var headers = Headers.init(allocator); defer headers.deinit(); try headers.append("foo", "bar"); try std.testing.expectEqualStrings(headers.getFirstValue("foo").?, "bar"); } -test "getFirstValue with multiple headers (bugfix regression test)" { +test "get with multiple headers (bugfix regression test)" { const allocator = std.testing.allocator; - var headers = Self.init(allocator); + var headers = Headers.init(allocator); defer headers.deinit(); try headers.append("foo", "bar"); try headers.append("bar", "baz"); - try std.testing.expectEqualStrings(headers.getFirstValue("bar").?, "baz"); + try std.testing.expectEqualStrings(headers.get("bar").?, "baz"); +} + +test "getAll" { + const allocator = std.testing.allocator; + var headers = Headers.init(allocator); + defer headers.deinit(); + try headers.append("foo", "bar"); + try headers.append("foo", "baz"); + try headers.append("bar", "qux"); + const all = headers.getAll("foo"); + defer allocator.free(all); + try std.testing.expectEqualSlices([]const u8, all, &[_][]const u8{ "bar", "baz" }); +} + +test "append too many headers" { + const allocator = std.testing.allocator; + var headers = Headers.init(allocator); + defer headers.deinit(); + for (0..25) |_| try headers.append("foo", "bar"); + + try std.testing.expectError(error.JetzigTooManyHeaders, headers.append("foo", "bar")); } test "case-insensitive matching" { const allocator = std.testing.allocator; - var headers = Self.init(allocator); + var headers = Headers.init(allocator); defer headers.deinit(); try headers.append("Content-Type", "bar"); try std.testing.expectEqualStrings(headers.getFirstValue("content-type").?, "bar"); @@ -112,7 +152,7 @@ test "case-insensitive matching" { test "iterator" { const allocator = std.testing.allocator; - var headers = Self.init(allocator); + var headers = Headers.init(allocator); defer headers.deinit(); try headers.append("foo", "bar"); @@ -129,7 +169,7 @@ test "iterator" { test "remove" { const allocator = std.testing.allocator; - var headers = Self.init(allocator); + var headers = Headers.init(allocator); defer headers.deinit(); try headers.append("foo", "baz"); try headers.append("foo", "qux"); @@ -141,7 +181,7 @@ test "remove" { test "stdHeaders" { const allocator = std.testing.allocator; - var headers = Self.init(allocator); + var headers = Headers.init(allocator); defer headers.deinit(); try headers.append("foo", "bar"); diff --git a/src/jetzig/http/Query.zig b/src/jetzig/http/Query.zig index c8f073b..4959d6e 100644 --- a/src/jetzig/http/Query.zig +++ b/src/jetzig/http/Query.zig @@ -33,18 +33,22 @@ pub fn parse(self: *Query) !void { while (pairs_it.next()) |pair| { var key_value_it = std.mem.splitScalar(u8, pair, '='); var count: u2 = 0; - var key: []const u8 = undefined; + var maybe_key: ?[]const u8 = null; var value: ?[]const u8 = null; while (key_value_it.next()) |key_or_value| { switch (count) { - 0 => key = key_or_value, + 0 => maybe_key = key_or_value, 1 => value = key_or_value, else => return error.JetzigQueryParseError, } count += 1; } - try self.query_items.append(.{ .key = key, .value = value }); + if (maybe_key) |key| { + if (key.len > 0) { + try self.query_items.append(.{ .key = key, .value = value }); + } + } } var params = try self.data.object(); @@ -120,8 +124,8 @@ test "simple query string" { defer query.deinit(); try query.parse(); - try std.testing.expectEqualStrings((try data.get("foo")).string.value, "bar"); - try std.testing.expectEqualStrings((try data.get("baz")).string.value, "qux"); + try std.testing.expectEqualStrings((data.get("foo")).?.string.value, "bar"); + try std.testing.expectEqualStrings((data.get("baz")).?.string.value, "qux"); } test "query string with array values" { @@ -134,7 +138,7 @@ test "query string with array values" { try query.parse(); - const value = try data.get("foo"); + const value = data.get("foo").?; switch (value.*) { .array => |array| { try std.testing.expectEqualStrings(array.get(0).?.string.value, "bar"); @@ -154,7 +158,7 @@ test "query string with mapping values" { try query.parse(); - const value = try data.get("foo"); + const value = data.get("foo").?; switch (value.*) { .object => |object| { try std.testing.expectEqualStrings(object.get("bar").?.string.value, "baz"); @@ -174,13 +178,13 @@ test "query string with param without value" { try query.parse(); - const foo = try data.get("foo"); + const foo = data.get("foo").?; try switch (foo.*) { .Null => {}, else => std.testing.expect(false), }; - const bar = try data.get("bar"); + const bar = data.get("bar").?; try switch (bar.*) { .Null => {}, else => std.testing.expect(false), diff --git a/src/jetzig/http/Request.zig b/src/jetzig/http/Request.zig index 00b2051..c250a92 100644 --- a/src/jetzig/http/Request.zig +++ b/src/jetzig/http/Request.zig @@ -2,7 +2,7 @@ const std = @import("std"); const jetzig = @import("../../jetzig.zig"); -const Self = @This(); +const Request = @This(); const default_content_type = "text/html"; pub const Method = enum { DELETE, GET, PATCH, POST, HEAD, PUT, CONNECT, OPTIONS, TRACE }; @@ -18,8 +18,8 @@ std_http_request: std.http.Server.Request, response: *jetzig.http.Response, status_code: jetzig.http.status_codes.StatusCode = undefined, response_data: *jetzig.data.Data, -query_data: *jetzig.data.Data, -query: *jetzig.http.Query, +query_params: ?*jetzig.http.Query = null, +query_body: ?*jetzig.http.Query = null, cookies: *jetzig.http.Cookies = undefined, session: *jetzig.http.Session = undefined, body: []const u8 = undefined, @@ -38,7 +38,7 @@ pub fn init( start_time: i128, std_http_request: std.http.Server.Request, response: *jetzig.http.Response, -) !Self { +) !Request { const method = switch (std_http_request.head.method) { .DELETE => Method.DELETE, .GET => Method.GET, @@ -55,11 +55,6 @@ pub fn init( const response_data = try allocator.create(jetzig.data.Data); response_data.* = jetzig.data.Data.init(allocator); - const query_data = try allocator.create(jetzig.data.Data); - query_data.* = jetzig.data.Data.init(allocator); - - const query = try allocator.create(jetzig.http.Query); - return .{ .allocator = allocator, .path = jetzig.http.Path.init(std_http_request.head.target), @@ -68,14 +63,12 @@ pub fn init( .server = server, .response = response, .response_data = response_data, - .query_data = query_data, - .query = query, .std_http_request = std_http_request, .start_time = start_time, }; } -pub fn deinit(self: *Self) void { +pub fn deinit(self: *Request) void { // self.session.deinit(); self.allocator.destroy(self.cookies); self.allocator.destroy(self.session); @@ -83,7 +76,7 @@ pub fn deinit(self: *Self) void { } /// Process request, read body if present, parse headers (TODO) -pub fn process(self: *Self) !void { +pub fn process(self: *Request) !void { var headers_it = self.std_http_request.iterateHeaders(); var cookie: ?[]const u8 = null; @@ -117,7 +110,7 @@ pub fn process(self: *Self) !void { } /// Set response headers, write response payload, and finalize the response. -pub fn respond(self: *Self) !void { +pub fn respond(self: *Request) !void { if (!self.processed) unreachable; var cookie_it = self.cookies.headerIterator(); @@ -143,7 +136,7 @@ pub fn respond(self: *Self) !void { /// Render a response. This function can only be called once per request (repeat calls will /// trigger an error). -pub fn render(self: *Self, status_code: jetzig.http.status_codes.StatusCode) jetzig.views.View { +pub fn render(self: *Request, status_code: jetzig.http.status_codes.StatusCode) jetzig.views.View { if (self.rendered) self.rendered_multiple = true; self.rendered = true; @@ -160,7 +153,7 @@ pub fn render(self: *Self, status_code: jetzig.http.status_codes.StatusCode) jet /// ``` /// The second argument must be `moved_permanently` or `found`. pub fn redirect( - self: *Self, + self: *Request, location: []const u8, redirect_status: enum { moved_permanently, found }, ) jetzig.views.View { @@ -188,7 +181,7 @@ pub fn redirect( /// * `Accept` header (`application/json` or `text/html`) /// * `Content-Type` header (`application/json` or `text/html`) /// * Fall back to default: HTML -pub fn requestFormat(self: *Self) jetzig.http.Request.Format { +pub fn requestFormat(self: *Request) jetzig.http.Request.Format { return self.extensionFormat() orelse self.acceptHeaderFormat() orelse self.contentTypeHeaderFormat() orelse @@ -197,7 +190,7 @@ pub fn requestFormat(self: *Self) jetzig.http.Request.Format { /// Set the layout for the current request/response. Use this to override a `pub const layout` /// declaration in a view, either in middleware or in a view function itself. -pub fn setLayout(self: *Self, layout: ?[]const u8) void { +pub fn setLayout(self: *Request, layout: ?[]const u8) void { if (layout) |layout_name| { self.layout = layout_name; self.layout_disabled = false; @@ -208,7 +201,7 @@ pub fn setLayout(self: *Self, layout: ?[]const u8) void { /// Derive a layout name from the current request if defined, otherwise from the route (if /// defined). -pub fn getLayout(self: *Self, route: *jetzig.views.Route) ?[]const u8 { +pub fn getLayout(self: *Request, route: *jetzig.views.Route) ?[]const u8 { if (self.layout_disabled) return null; if (self.layout) |capture| return capture; if (route.layout) |capture| return capture; @@ -218,7 +211,7 @@ pub fn getLayout(self: *Self, route: *jetzig.views.Route) ?[]const u8 { /// Shortcut for `request.headers.getFirstValue`. Returns the first matching value for a given /// header name or `null` if not found. Header names are case-insensitive. -pub fn getHeader(self: *Self, key: []const u8) ?[]const u8 { +pub fn getHeader(self: *Request, key: []const u8) ?[]const u8 { return self.headers.getFirstValue(key); } @@ -227,7 +220,7 @@ pub fn getHeader(self: *Self, key: []const u8) ?[]const u8 { /// Note that query parameters are supported for JSON requests if no request body is present, /// otherwise the parsed JSON request body will take precedence and query parameters will be /// ignored. -pub fn params(self: *Self) !*jetzig.data.Value { +pub fn params(self: *Request) !*jetzig.data.Value { if (!self.processed) unreachable; switch (self.requestFormat()) { @@ -244,35 +237,45 @@ pub fn params(self: *Self) !*jetzig.data.Value { }; return data.value.?; }, - .HTML, .UNKNOWN => return self.queryParams(), + .HTML, .UNKNOWN => return self.parseQuery(), } } -fn queryParams(self: *Self) !*jetzig.data.Value { - if (!try self.parseQueryString()) { - self.query.data = try self.allocator.create(jetzig.data.Data); - self.query.data.* = jetzig.data.Data.init(self.allocator); - _ = try self.query.data.object(); - } - return self.query.data.value.?; +/// Return a `*Value` representing request parameters. This function **always** returns the +/// parsed query string and never the request body. +pub fn queryParams(self: *Request) !*jetzig.data.Value { + if (self.query_params) |parsed| return parsed.data.value.?; + + const data = try self.allocator.create(jetzig.data.Data); + data.* = jetzig.data.Data.init(self.allocator); + self.query_params = try self.allocator.create(jetzig.http.Query); + self.query_params.?.* = jetzig.http.Query.init( + self.allocator, + self.path.query orelse "", + data, + ); + try self.query_params.?.parse(); + return self.query_params.?.data.value.?; } -fn parseQueryString(self: *Self) !bool { - const data: ?[]const u8 = if (self.body.len > 0) self.body else self.path.query; - if (data) |query| { - self.query.* = jetzig.http.Query.init( - self.allocator, - query, - self.query_data, - ); - try self.query.parse(); - return true; - } +// Parses request body as params if present, otherwise delegates to `queryParams`. +fn parseQuery(self: *Request) !*jetzig.data.Value { + if (self.body.len == 0) return try self.queryParams(); + if (self.query_body) |parsed| return parsed.data.value.?; - return false; + const data = try self.allocator.create(jetzig.data.Data); + data.* = jetzig.data.Data.init(self.allocator); + self.query_body = try self.allocator.create(jetzig.http.Query); + self.query_body.?.* = jetzig.http.Query.init( + self.allocator, + self.body, + data, + ); + try self.query_body.?.parse(); + return self.query_body.?.data.value.?; } -fn extensionFormat(self: *Self) ?jetzig.http.Request.Format { +fn extensionFormat(self: *Request) ?jetzig.http.Request.Format { const extension = self.path.extension orelse return null; if (std.mem.eql(u8, extension, ".html")) { @@ -284,7 +287,7 @@ fn extensionFormat(self: *Self) ?jetzig.http.Request.Format { } } -pub fn acceptHeaderFormat(self: *Self) ?jetzig.http.Request.Format { +pub fn acceptHeaderFormat(self: *Request) ?jetzig.http.Request.Format { const acceptHeader = self.getHeader("Accept"); if (acceptHeader) |item| { @@ -295,7 +298,7 @@ pub fn acceptHeaderFormat(self: *Self) ?jetzig.http.Request.Format { return null; } -pub fn contentTypeHeaderFormat(self: *Self) ?jetzig.http.Request.Format { +pub fn contentTypeHeaderFormat(self: *Request) ?jetzig.http.Request.Format { const acceptHeader = self.getHeader("content-type"); if (acceptHeader) |item| { @@ -306,7 +309,7 @@ pub fn contentTypeHeaderFormat(self: *Self) ?jetzig.http.Request.Format { return null; } -pub fn hash(self: *Self) ![]const u8 { +pub fn hash(self: *Request) ![]const u8 { return try std.fmt.allocPrint( self.allocator, "{s}-{s}-{s}", @@ -314,7 +317,7 @@ pub fn hash(self: *Self) ![]const u8 { ); } -pub fn fmtMethod(self: *const Self, colorized: bool) []const u8 { +pub fn fmtMethod(self: *const Request, colorized: bool) []const u8 { if (!colorized) return @tagName(self.method); return switch (self.method) { @@ -331,7 +334,7 @@ pub fn fmtMethod(self: *const Self, colorized: bool) []const u8 { /// Format a status code appropriately for the current request format. /// e.g. `.HTML` => `404 Not Found` /// `.JSON` => `{ "message": "Not Found", "status": "404" }` -pub fn formatStatus(self: *Self, status_code: jetzig.http.StatusCode) ![]const u8 { +pub fn formatStatus(self: *Request, status_code: jetzig.http.StatusCode) ![]const u8 { const status = jetzig.http.status_codes.get(status_code); return switch (self.requestFormat()) { @@ -344,7 +347,7 @@ pub fn formatStatus(self: *Self, status_code: jetzig.http.StatusCode) ![]const u } pub fn setResponse( - self: *Self, + self: *Request, rendered_view: jetzig.http.Server.RenderedView, options: struct { content_type: ?[]const u8 = null }, ) void { @@ -357,7 +360,7 @@ pub fn setResponse( } // Determine if a given route matches the current request. -pub fn match(self: *Self, route: jetzig.views.Route) !bool { +pub fn match(self: *Request, route: jetzig.views.Route) !bool { return switch (self.method) { .GET => switch (route.action) { .index => self.isMatch(.exact, route), @@ -384,7 +387,7 @@ pub fn match(self: *Self, route: jetzig.views.Route) !bool { }; } -fn isMatch(self: *Self, match_type: enum { exact, resource_id }, route: jetzig.views.Route) bool { +fn isMatch(self: *Request, match_type: enum { exact, resource_id }, route: jetzig.views.Route) bool { const path = switch (match_type) { .exact => self.path.base_path, .resource_id => self.path.directory,