diff --git a/src/msgpack.zig b/src/msgpack.zig index a6ffdf4..5cb353a 100644 --- a/src/msgpack.zig +++ b/src/msgpack.zig @@ -20,6 +20,14 @@ const little_endian = switch (current_zig.minor) { else => @compileError("not support current version zig"), }; +// Constants for improved code readability +const MAX_POSITIVE_FIXINT: u8 = 0x7f; +const MIN_NEGATIVE_FIXINT: i8 = -32; +const MAX_FIXSTR_LEN: u8 = 31; +const MAX_FIXARRAY_LEN: u8 = 15; +const MAX_FIXMAP_LEN: u8 = 15; +const TIMESTAMP_EXT_TYPE: i8 = -1; + /// the Str Type pub const Str = struct { str: []const u8, @@ -158,15 +166,17 @@ pub const Payload = union(enum) { if (self.* != .map) { return Errors.NotMap; } - // TODO: This maybe memory leak - const old_key = self.map.getKey(key); - if (old_key) |old_key_ptr| { - // if the key is already in map, free the old key - self.map.allocator.free(old_key_ptr); + + // Check if the key already exists using getKeyPtr + if (self.map.getKeyPtr(key)) |existing_key| { + // Key exists, use the existing allocated key + try self.map.put(existing_key.*, val); + } else { + // Key doesn't exist, create a new key + const new_key = try self.map.allocator.alloc(u8, key.len); + @memcpy(new_key, key); + try self.map.put(new_key, val); } - const new_key = try self.map.allocator.alloc(u8, key.len); - @memcpy(new_key, key); - try self.map.put(new_key, val); } /// get a NIL payload @@ -206,9 +216,9 @@ pub const Payload = union(enum) { /// get a str payload pub fn strToPayload(val: []const u8, allocator: Allocator) !Payload { - // alloca memory + // allocate memory const new_str = try allocator.alloc(u8, val.len); - // copy the val + // copy the value @memcpy(new_str, val); return Payload{ .str = wrapStr(new_str), @@ -217,9 +227,9 @@ pub const Payload = union(enum) { /// get a bin payload pub fn binToPayload(val: []const u8, allocator: Allocator) !Payload { - // alloca memory + // allocate memory const new_bin = try allocator.alloc(u8, val.len); - // copy the val + // copy the value @memcpy(new_bin, val); return Payload{ .bin = wrapBin(new_bin), @@ -246,9 +256,9 @@ pub const Payload = union(enum) { /// get an ext payload pub fn extToPayload(t: i8, data: []const u8, allocator: Allocator) !Payload { - // alloca memory + // allocate memory const new_data = try allocator.alloc(u8, data.len); - // copy the val + // copy the value @memcpy(new_data, data); return Payload{ .ext = wrapEXT(t, new_data), @@ -269,7 +279,7 @@ pub const Payload = union(enum) { }; } - /// free the all memeory for this payload and sub payloads + /// free all memory for this payload and sub payloads /// the allocator is payload's allocator pub fn free(self: Payload, allocator: Allocator) void { switch (self) { @@ -310,8 +320,8 @@ pub const Payload = union(enum) { } } - /// get a i64 value from payload - /// Note: if the payload is not a int or the value is too large, it will return MsGPackError.INVALID_TYPE + /// get an i64 value from payload + /// Note: if the payload is not an int or the value is too large, it will return MsGPackError.INVALID_TYPE pub fn getInt(self: Payload) !i64 { return switch (self) { .int => |val| val, @@ -319,14 +329,14 @@ pub const Payload = union(enum) { if (val <= std.math.maxInt(i64)) { return @intCast(val); } - // TODO: we can not return this error + // Value exceeds i64 range return MsGPackError.INVALID_TYPE; }, else => return MsGPackError.INVALID_TYPE, }; } - /// get a u64 value from payload + /// get an u64 value from payload /// Note: if the payload is not a uint or the value is negative, it will return MsGPackError.INVALID_TYPE pub fn getUint(self: Payload) !u64 { return switch (self) { @@ -334,7 +344,7 @@ pub const Payload = union(enum) { if (val >= 0) { return @intCast(val); } - // TODO: we can not return this error + // Negative values cannot be converted to u64 return MsGPackError.INVALID_TYPE; }, .uint => |val| val, @@ -478,7 +488,7 @@ pub fn Pack( /// write positive fix int fn writePfixInt(self: Self, val: u8) !void { - if (val <= 0x7f) { + if (val <= MAX_POSITIVE_FIXINT) { try self.writeByte(val); } else { return MsGPackError.INPUT_VALUE_TOO_LARGE; @@ -536,7 +546,7 @@ pub fn Pack( /// write negative fix int fn writeNfixInt(self: Self, val: i8) !void { - if (val >= -32 and val <= -1) { + if (val >= MIN_NEGATIVE_FIXINT and val <= -1) { try self.writeByte(@bitCast(val)); } else { return MsGPackError.INPUT_VALUE_TOO_LARGE; @@ -594,7 +604,7 @@ pub fn Pack( /// write uint fn writeUint(self: Self, val: u64) !void { - if (val <= 0x7f) { + if (val <= MAX_POSITIVE_FIXINT) { try self.writePfixInt(@intCast(val)); } else if (val <= 0xff) { try self.writeU8(@intCast(val)); @@ -611,7 +621,7 @@ pub fn Pack( fn writeInt(self: Self, val: i64) !void { if (val >= 0) { try self.writeUint(@intCast(val)); - } else if (val >= -32) { + } else if (val >= MIN_NEGATIVE_FIXINT) { try self.writeNfixInt(@intCast(val)); } else if (val >= -128) { try self.writeI8(@intCast(val)); @@ -672,7 +682,7 @@ pub fn Pack( /// write fix str fn writeFixStr(self: Self, str: []const u8) !void { const len = str.len; - if (len > 0x1f) { + if (len > MAX_FIXSTR_LEN) { return MsGPackError.STR_DATA_LENGTH_TOO_LONG; } const header: u8 = @intFromEnum(Markers.FIXSTR) + @as(u8, @intCast(len)); @@ -738,7 +748,7 @@ pub fn Pack( /// write str fn writeStr(self: Self, str: Str) !void { const len = str.value().len; - if (len <= 0x1f) { + if (len <= MAX_FIXSTR_LEN) { try self.writeFixStr(str.value()); } else if (len <= 0xff) { try self.writeStr8(str.value()); @@ -897,13 +907,12 @@ pub fn Pack( /// write timestamp fn writeTimestamp(self: Self, timestamp: Timestamp) !void { // According to MessagePack spec, timestamp uses extension type -1 - const TIMESTAMP_TYPE: i8 = -1; // timestamp 32 format: seconds fit in 32-bit unsigned int and nanoseconds is 0 if (timestamp.nanoseconds == 0 and timestamp.seconds >= 0 and timestamp.seconds <= 0xffffffff) { var data: [4]u8 = undefined; std.mem.writeInt(u32, &data, @intCast(timestamp.seconds), big_endian); - const ext = EXT{ .type = TIMESTAMP_TYPE, .data = &data }; + const ext = EXT{ .type = TIMESTAMP_EXT_TYPE, .data = &data }; try self.writeExt(ext); return; } @@ -913,7 +922,7 @@ pub fn Pack( const data64: u64 = (@as(u64, timestamp.nanoseconds) << 34) | @as(u64, @intCast(timestamp.seconds)); var data: [8]u8 = undefined; std.mem.writeInt(u64, &data, data64, big_endian); - const ext = EXT{ .type = TIMESTAMP_TYPE, .data = &data }; + const ext = EXT{ .type = TIMESTAMP_EXT_TYPE, .data = &data }; try self.writeExt(ext); return; } @@ -923,7 +932,7 @@ pub fn Pack( var data: [12]u8 = undefined; std.mem.writeInt(u32, data[0..4], timestamp.nanoseconds, big_endian); std.mem.writeInt(i64, data[4..12], timestamp.seconds, big_endian); - const ext = EXT{ .type = TIMESTAMP_TYPE, .data = &data }; + const ext = EXT{ .type = TIMESTAMP_EXT_TYPE, .data = &data }; try self.writeExt(ext); return; } @@ -957,7 +966,7 @@ pub fn Pack( }, .arr => |arr| { const len = arr.len; - if (len <= 0xf) { + if (len <= MAX_FIXARRAY_LEN) { const header: u8 = @intFromEnum(Markers.FIXARRAY) + @as(u8, @intCast(len)); try self.writeU8Value(header); } else if (len <= 0xffff) { @@ -975,7 +984,7 @@ pub fn Pack( }, .map => |map| { const len = map.count(); - if (len <= 0xf) { + if (len <= MAX_FIXMAP_LEN) { const header: u8 = @intFromEnum(Markers.FIXMAP) + @as(u8, @intCast(len)); try self.writeU8Value(header); } else if (len <= 0xffff) { @@ -1373,8 +1382,6 @@ pub fn Pack( /// read ext value or timestamp if it's timestamp type (-1) fn readExtValueOrTimestamp(self: Self, marker: Markers, allocator: Allocator) !Payload { - const TIMESTAMP_TYPE: i8 = -1; - // First, check if this could be a timestamp format if (marker == .FIXEXT4 or marker == .FIXEXT8 or marker == .EXT8) { // Read and check length for EXT8 @@ -1397,7 +1404,7 @@ pub fn Pack( // Read the type const ext_type = try self.readI8Value(); - if (ext_type == TIMESTAMP_TYPE) { + if (ext_type == TIMESTAMP_EXT_TYPE) { // This is a timestamp if (marker == .FIXEXT4) { // timestamp 32 @@ -1430,13 +1437,11 @@ pub fn Pack( /// try to read timestamp from ext data, return error if not timestamp fn tryReadTimestamp(self: Self, marker: Markers, _: Allocator) !Timestamp { - const TIMESTAMP_TYPE: i8 = -1; - switch (marker) { .FIXEXT4 => { // timestamp 32 format const ext_type = try self.readI8Value(); - if (ext_type != TIMESTAMP_TYPE) { + if (ext_type != TIMESTAMP_EXT_TYPE) { return MsGPackError.INVALID_TYPE; } const seconds = try self.readU32Value(); @@ -1445,7 +1450,7 @@ pub fn Pack( .FIXEXT8 => { // timestamp 64 format const ext_type = try self.readI8Value(); - if (ext_type != TIMESTAMP_TYPE) { + if (ext_type != TIMESTAMP_EXT_TYPE) { return MsGPackError.INVALID_TYPE; } const data64 = try self.readU64Value(); @@ -1460,7 +1465,7 @@ pub fn Pack( return MsGPackError.INVALID_TYPE; } const ext_type = try self.readI8Value(); - if (ext_type != TIMESTAMP_TYPE) { + if (ext_type != TIMESTAMP_EXT_TYPE) { return MsGPackError.INVALID_TYPE; } const nanoseconds = try self.readU32Value(); @@ -1475,13 +1480,11 @@ pub fn Pack( /// read timestamp from ext data fn readTimestamp(self: Self, marker: Markers, _: Allocator) !Timestamp { - const TIMESTAMP_TYPE: i8 = -1; - switch (marker) { .FIXEXT4 => { // timestamp 32 format const ext_type = try self.readI8Value(); - if (ext_type != TIMESTAMP_TYPE) { + if (ext_type != TIMESTAMP_EXT_TYPE) { return MsGPackError.INVALID_TYPE; } const seconds = try self.readU32Value(); @@ -1490,7 +1493,7 @@ pub fn Pack( .FIXEXT8 => { // timestamp 64 format const ext_type = try self.readI8Value(); - if (ext_type != TIMESTAMP_TYPE) { + if (ext_type != TIMESTAMP_EXT_TYPE) { return MsGPackError.INVALID_TYPE; } const data64 = try self.readU64Value(); @@ -1505,7 +1508,7 @@ pub fn Pack( return MsGPackError.INVALID_TYPE; } const ext_type = try self.readI8Value(); - if (ext_type != TIMESTAMP_TYPE) { + if (ext_type != TIMESTAMP_EXT_TYPE) { return MsGPackError.INVALID_TYPE; } const nanoseconds = try self.readU32Value(); diff --git a/src/test.zig b/src/test.zig index 4abd311..7e59723 100644 --- a/src/test.zig +++ b/src/test.zig @@ -1878,3 +1878,246 @@ test "timestamp precision and conversion" { const expected4 = -0.5; try expect(@abs(float_val4 - expected4) < 0.000000001); } + +// ============================================================================ +// Additional tests from test_additional.zig +// ============================================================================ + +// Test minimal encoding principle (serializers SHOULD use the smallest format) +test "minimal encoding principle" { + var arr: [1000]u8 = std.mem.zeroes([1000]u8); + var write_buffer = fixedBufferStream(&arr); + var read_buffer = fixedBufferStream(&arr); + var p = pack.init(&write_buffer, &read_buffer); + + // Test small integers use positive fixint + try p.write(.{ .uint = 127 }); + try expect(arr[0] == 0x7f); // Should use positive fixint, not uint8 + + // Reset + arr = std.mem.zeroes([1000]u8); + write_buffer = fixedBufferStream(&arr); + p = pack.init(&write_buffer, &read_buffer); + + // Test small negative numbers use negative fixint + try p.write(.{ .int = -1 }); + try expect(arr[0] == 0xff); // Should use negative fixint + + // Reset + arr = std.mem.zeroes([1000]u8); + write_buffer = fixedBufferStream(&arr); + p = pack.init(&write_buffer, &read_buffer); + + // Test short strings use fixstr + const short_str = try Payload.strToPayload("hello", allocator); + defer short_str.free(allocator); + try p.write(short_str); + try expect((arr[0] & 0xe0) == 0xa0); // fixstr format + try expect((arr[0] & 0x1f) == 5); // length is 5 +} + +// Test all positive fixint values (0-127) +test "all positive fixint values comprehensive" { + var arr: [256]u8 = std.mem.zeroes([256]u8); + var write_buffer = fixedBufferStream(&arr); + var read_buffer = fixedBufferStream(&arr); + var p = pack.init(&write_buffer, &read_buffer); + + // Write all positive fixint values + for (0..128) |i| { + try p.write(.{ .uint = i }); + } + + // Verify all values use single-byte encoding + for (0..128) |i| { + try expect(arr[i] == i); + } + + // Reset read buffer and verify read values + read_buffer = fixedBufferStream(&arr); + p = pack.init(&write_buffer, &read_buffer); + + for (0..128) |i| { + const val = try p.read(allocator); + defer val.free(allocator); + try expect(val.uint == i); + } +} + +// Test deterministic serialization for maps (useful for hashing scenarios) +test "deterministic serialization for maps" { + var arr1: [1000]u8 = std.mem.zeroes([1000]u8); + var arr2: [1000]u8 = std.mem.zeroes([1000]u8); + + // Create two maps with same content but different insertion order + var map1 = Payload.mapPayload(allocator); + defer map1.free(allocator); + try map1.mapPut("a", Payload.intToPayload(1)); + try map1.mapPut("b", Payload.intToPayload(2)); + try map1.mapPut("c", Payload.intToPayload(3)); + + var map2 = Payload.mapPayload(allocator); + defer map2.free(allocator); + try map2.mapPut("c", Payload.intToPayload(3)); + try map2.mapPut("a", Payload.intToPayload(1)); + try map2.mapPut("b", Payload.intToPayload(2)); + + var write_buffer1 = fixedBufferStream(&arr1); + var read_buffer1 = fixedBufferStream(&arr1); + var p1 = pack.init(&write_buffer1, &read_buffer1); + try p1.write(map1); + + var write_buffer2 = fixedBufferStream(&arr2); + var read_buffer2 = fixedBufferStream(&arr2); + var p2 = pack.init(&write_buffer2, &read_buffer2); + try p2.write(map2); + + // Note: Current implementation may not guarantee order consistency + // This is an area for potential improvement +} + +// Test bin and str type compatibility (cross-version compatibility) +test "bin and str type compatibility" { + var arr: [1000]u8 = std.mem.zeroes([1000]u8); + var write_buffer = fixedBufferStream(&arr); + var read_buffer = fixedBufferStream(&arr); + var p = pack.init(&write_buffer, &read_buffer); + + // Test binary data uses bin format + var binary_data = [_]u8{ 0xff, 0xfe, 0xfd, 0xfc, 0xfb }; + try p.write(.{ .bin = msgpack.wrapBin(&binary_data) }); + + // Verify bin8 format (0xc4) is used + try expect(arr[0] == 0xc4); + try expect(arr[1] == 5); // length + + // Test UTF-8 strings use str format + arr = std.mem.zeroes([1000]u8); + write_buffer = fixedBufferStream(&arr); + p = pack.init(&write_buffer, &read_buffer); + + const utf8_str = try Payload.strToPayload("Hello 世界", allocator); + defer utf8_str.free(allocator); + try p.write(utf8_str); + + // Verify str format is used + try expect((arr[0] & 0xe0) == 0xa0 or arr[0] == 0xd9); // fixstr or str8 +} + +// Test extension type reserved range +test "extension type reserved range" { + var arr: [1000]u8 = std.mem.zeroes([1000]u8); + var write_buffer = fixedBufferStream(&arr); + var read_buffer = fixedBufferStream(&arr); + var p = pack.init(&write_buffer, &read_buffer); + + // Test application-defined types (0-127) + var app_data = [_]u8{ 0x01, 0x02 }; + try p.write(.{ .ext = msgpack.wrapEXT(0, &app_data) }); + try p.write(.{ .ext = msgpack.wrapEXT(127, &app_data) }); + + // Test predefined types (-128 to -1) + try p.write(.{ .ext = msgpack.wrapEXT(-128, &app_data) }); + // -1 is timestamp, already covered in other tests + try p.write(.{ .ext = msgpack.wrapEXT(-2, &app_data) }); + + // Read back and verify type values remain correct + read_buffer = fixedBufferStream(&arr); + p = pack.init(&write_buffer, &read_buffer); + + const val1 = try p.read(allocator); + defer val1.free(allocator); + try expect(val1.ext.type == 0); + + const val2 = try p.read(allocator); + defer val2.free(allocator); + try expect(val2.ext.type == 127); + + const val3 = try p.read(allocator); + defer val3.free(allocator); + try expect(val3.ext.type == -128); + + const val4 = try p.read(allocator); + defer val4.free(allocator); + try expect(val4.ext.type == -2); +} + +// Test sequential read write multiple objects +test "sequential read write multiple objects" { + var arr: [10000]u8 = std.mem.zeroes([10000]u8); + var write_buffer = fixedBufferStream(&arr); + var read_buffer = fixedBufferStream(&arr); + var p = pack.init(&write_buffer, &read_buffer); + + // Write multiple types of objects + try p.write(Payload.nilToPayload()); + try p.write(Payload.boolToPayload(true)); + try p.write(Payload.intToPayload(-42)); + try p.write(Payload.uintToPayload(42)); + try p.write(Payload.floatToPayload(3.14)); + + const str = try Payload.strToPayload("test", allocator); + defer str.free(allocator); + try p.write(str); + + var bin_data = [_]u8{ 1, 2, 3 }; + try p.write(.{ .bin = msgpack.wrapBin(&bin_data) }); + + var test_arr = try Payload.arrPayload(2, allocator); + defer test_arr.free(allocator); + try test_arr.setArrElement(0, Payload.intToPayload(1)); + try test_arr.setArrElement(1, Payload.intToPayload(2)); + try p.write(test_arr); + + var test_map = Payload.mapPayload(allocator); + defer test_map.free(allocator); + try test_map.mapPut("key", Payload.intToPayload(100)); + try p.write(test_map); + + const ts = Payload.timestampFromSeconds(1000000); + try p.write(ts); + + // Read back all objects and verify + read_buffer = fixedBufferStream(&arr); + p = pack.init(&write_buffer, &read_buffer); + + const v1 = try p.read(allocator); + defer v1.free(allocator); + try expect(v1 == .nil); + + const v2 = try p.read(allocator); + defer v2.free(allocator); + try expect(v2.bool == true); + + const v3 = try p.read(allocator); + defer v3.free(allocator); + try expect(v3.int == -42); + + const v4 = try p.read(allocator); + defer v4.free(allocator); + try expect(v4.uint == 42); + + const v5 = try p.read(allocator); + defer v5.free(allocator); + try expect(@abs(v5.float - 3.14) < 0.01); + + const v6 = try p.read(allocator); + defer v6.free(allocator); + try expect(std.mem.eql(u8, v6.str.value(), "test")); + + const v7 = try p.read(allocator); + defer v7.free(allocator); + try expect(v7.bin.value().len == 3); + + const v8 = try p.read(allocator); + defer v8.free(allocator); + try expect((try v8.getArrLen()) == 2); + + const v9 = try p.read(allocator); + defer v9.free(allocator); + try expect(v9.map.count() == 1); + + const v10 = try p.read(allocator); + defer v10.free(allocator); + try expect(v10.timestamp.seconds == 1000000); +}