std.json: support field aliases #8987

travisstaloch · 2021-06-04T06:07:49Z

if a struct T passed to parse(T, ...) contains a pub const __tags of the form
shown below, these aliases will be used as the json keys instead of the
field name.
adds std.mem.{indexOfSliceScalar, indexOfSliceScalarPos} which support
finding aliases. adding these was my workaround to a compiler bug
encountered when using a for loop for the same purpose.

    const S = struct {
        value: u8,
        pub const __tags = .{
            .value = .{ .alias = "__value" },
        };
    };

The names __tags and alias were chosen by me ad-hoc and may be easily
changed.
Inspired by #1099 #1099 (comment)

- if a struct T passed to parse(T) contains a pub const __tags of the form shown below, these aliases will be used as the json keys instead of the field name. - adds std.mem.{indexOfSliceScalar, indexOfSliceScalarPos} which support finding aliases. adding these was my workaround to a compiler bug encountered when using a for loop for the same purpose. The names __tags and alias were chosen by me ad-hoc and may be easily changed.

travisstaloch · 2021-06-04T06:09:51Z

@SpexGuy suspects that this is failing due to significant comptime usage and may have to wait for stage 2.

travisstaloch · 2021-06-04T06:13:51Z

I messed up my rebase in previous pr. Forgive me as I've closed it and reopened this after mikdusan suggested that the real issue was needing a rebase. Maybe this will pass ci now...

andrewrk · 2021-06-04T08:22:13Z

Sorry about that. With self hosted you should be able to comptime to your heart's content. I am confident we can ship it by zig 0.9.0.

daurnimator

I'm not sure this is the best way to accomplish this feature: I was thinking it would be better to provide on a per parse/stringify invocation via an option that contains a map of overrides

lib/std/json.zig

travisstaloch · 2021-06-04T21:32:50Z

@andrewrk looking forward to 0.9 when comptime will go brrr 👍.

Should i leave this pr open then after everything else is resolved? Or close it and plan to re-open / make a new pr after 0.9?

andrewrk · 2021-06-04T21:34:35Z

I'd like to reserve the PR queue for "ready to be reviewed & merged". If something can't be merge-ready due to being blocked by other (not immediately solvable) issues, then it should be an issue, and then resurrected as a PR when the time comes.

travisstaloch · 2021-06-04T23:54:11Z

Sounds good. I opened #8993 as a reminder to revisit after 0.9. Will close this once other issues with this pr are resolved.

travisstaloch · 2021-06-08T03:46:30Z

@daurnimator i made some changes. i'm not sure what needs to be done to support escapes. are you saying that the escaped version of stringToken.slice(tokens.slice, tokens.i - 1) should be escaped before checking for a matching alias? any advice how i could do that?

daurnimator · 2021-06-08T04:16:16Z

i'm not sure what needs to be done to support escapes. are you saying that the escaped version of stringToken.slice(tokens.slice, tokens.i - 1) should be escaped before checking for a matching alias? any advice how i could do that?

I'm saying you need to make sure {"foo": "bar"} and {"f\x6fo":"bar"} are treated the same.
See the line just below where you edited for how we do that:

(stringToken.escapes == .None and mem.eql(u8, field.name, key_source_slice)) or (stringToken.escapes == .Some and (field.name.len == stringToken.decodedLength() and encodesTo(field.name, key_source_slice)))

i.e. if stringToken.escapes == .None we can match against the field name directly; otherwise we need to use a different equality check. We have the encodesTo for such situations.

Note that to do this you'll want to change your implementation to do most of the work inside the inline for (structInfo.fields) |field, i| { loop

Also note typo in your commit message: s/PaseOptions/ParseOptions

- introduces `ParseOptions.field_aliases` as a `[]const [2][]const u8`. usage becomes: `.{ .field_aliases = &.{.{ "value", "__value" }} }` - buildFieldAliases() now returns a `StringHashMapUnmanaged([]const u8)` and no longer requires excessive comptime memory.

travisstaloch · 2021-06-08T04:55:16Z

thank you 👍. so i added this test which i expected to fail, but its passing:

test "field alias escapes" {
    const S = struct { foo: u8 };
    const text =
        "{\"__f\x6fo\": 42}";

    const s = try parse(S, &TokenStream.init(text), .{
        .field_aliases = &.{.{ "foo", "__foo" }},
        .allocator = std.testing.allocator,
    });
    try testing.expectEqual(@as(u8, 42), s.foo);
}

not sure what to make of this... 🤔

am i testing the wrong thing? i tried this which also passed:

test "field alias escapes" {
    const S = struct { foo: u8 };
    const text =
        "{\"__foo\": 42}";

    const s = try parse(S, &TokenStream.init(text), .{
        .field_aliases = &.{.{ "foo", "__f\x6fo" }},
        .allocator = std.testing.allocator,
    });
    try testing.expectEqual(@as(u8, 42), s.foo);
}

can you think of a test case which i should include to make this fail?

daurnimator · 2021-06-08T07:02:23Z

@travisstaloch you have 2 layers of escaping: \x6f is already understood by zig, you need to use zig \\ strings or write "\\x6f"

lib/std/json.zig

travisstaloch · 2021-06-08T07:32:06Z

@travisstaloch you have 2 layers of escaping: \x6f is already understood by zig, you need to use zig \\ strings or write "\\x6f"

Ok, well I was initially using multiline escapes, but ran into this problem. Turns out std.json can't even parse this and is giving the same error i was seeing:

const std = @import("std");

test "field alias escapes" {
    const S = struct { foo: u8 };
    const text =
        \\{"f\x6fo": 42}
    ;

    const s = try std.json.parse(S, &std.json.TokenStream.init(text), .{});
    try std.testing.expectEqual(@as(u8, 42), s.foo);
}

gives error:

$ zig version
0.9.0-dev.21+7462b0e5b

$ zig test /tmp/test.zig 
test "field alias escapes"... FAIL (InvalidEscapeCharacter)
~/Documents/Code/zig/zig/build/lib/zig/std/json.zig:753:21: 0x211714 in std.json.StreamingParser.transition (test)
                    return error.InvalidEscapeCharacter;
                    ^
~/Documents/Code/zig/zig/build/lib/zig/std/json.zig:284:13: 0x20bc86 in std.json.StreamingParser.feed (test)
        if (try p.transition(c, token1)) {
            ^
~/Documents/Code/zig/zig/build/lib/zig/std/json.zig:1122:13: 0x20abf0 in std.json.TokenStream.next (test)
            try self.parser.feed(self.slice[self.i], &t1, &t2);
            ^
~/Documents/Code/zig/zig/build/lib/zig/std/json.zig:1628:26: 0x20b089 in std.json.parseInternal (test)
                switch ((try tokens.next()) orelse return error.UnexpectedEndOfJson) {
                         ^
~/Documents/Code/zig/zig/build/lib/zig/std/json.zig:1795:15: 0x20985e in std.json.parse (test)
    const r = try parseInternal(T, token, tokens, options);
              ^
/tmp/test.zig:9:15: 0x207d31 in test "field alias escapes" (test)
    const s = try std.json.parse(S, &std.json.TokenStream.init(text), .{});

is this an issue with std.json or an invalid input? maybe i'm missing something?

daurnimator · 2021-06-08T08:11:09Z

is this an issue with std.json or an invalid input? maybe i'm missing something?

was just me being wrong :P json doesn't support \x escapes. Instead of \x6f try \u006f

travisstaloch · 2021-06-08T08:35:32Z

Ok good. So does this look like the test i'm trying to make pass? I want to be sure i'm testing the right thing.

test "field alias escapes" {
    const S = struct { foo: u8 };
    const text =
        \\{"__f\u006fo": 42}
    ;
    const s = try parse(S, &TokenStream.init(text), .{
        .field_aliases = &.{.{ .field = "foo", .alias = 
        \\__f\u006fo 
        }},
        .allocator = std.testing.allocator,
    });
    try testing.expectEqual(@as(u8, 42), s.foo);
}

daurnimator · 2021-06-08T09:41:13Z

no you want to set the alias to just __foo. This should pass:

test "field alias escapes" {
    const S = struct { foo: u8 };
    const text =
        \\{"__f\u006fo": 42}
    ;
    const s = try parse(S, &TokenStream.init(text), .{
        .field_aliases = &.{.{ .field = "foo", .alias =  "__foo" }},
        .allocator = std.testing.allocator,
    });
    try testing.expectEqual(@as(u8, 42), s.foo);
}

- now using correct escape sequence and key - attempt to reformat and add a few comments to `inline for (structInfo.fields) |field, i|` loop body

comments addressed

- now maps from field_name => alias_name - this unifies and improves searching for aliases. - this also fixes missing check for field.name equality. - add test coverage: false positive matches

daurnimator

I don't think you need an allocator: you can bound the size of the field aliases by the number of fields in the type, which means it can be on the stack.

daurnimator · 2021-06-09T02:14:23Z

lib/std/json.zig

@@ -1466,6 +1466,7 @@ pub const ParseOptions = struct {
    ignore_unknown_fields: bool = false,

    allow_trailing_data: bool = false,
+    field_aliases: ?[]const struct { field: []const u8, alias: []const u8 } = null,


Doesn't need to be null-able: just use the empty list.

daurnimator · 2021-06-09T02:14:53Z

lib/std/json.zig

@@ -1466,6 +1466,7 @@ pub const ParseOptions = struct {
    ignore_unknown_fields: bool = false,

    allow_trailing_data: bool = false,
+    field_aliases: ?[]const struct { field: []const u8, alias: []const u8 } = null,


Add an empty line between fields here

Addressed all of these in latest commit.

travisstaloch · 2021-06-09T02:32:49Z

I don't think you need an allocator: you can bound the size of the field aliases by the number of fields in the type, which means it can be on the stack.

the allocator is only needed for the StringHashMapUnmanaged returned by buildFieldAliases(). how could i initialize this on the stack? just pass in a fixedBufferAllocator?

travisstaloch · 2021-06-09T02:56:56Z

this makes the tests pass. not sure if structInfo.fields.len * 512 its a good hueristic. its the smallest power of 2 that passes. does this seem acceptable?

            var buf: [structInfo.fields.len * (1 << 9)]u8 = undefined;
            var fba = std.heap.FixedBufferAllocator.init(&buf);
            var field_alias_map = try buildFieldAliasMap(T, options, &fba.allocator);

travisstaloch · 2021-06-09T11:36:18Z

continuous-integration/drone/pr and ziglang.zig ci jobs both passed before i got tired waiting to see if 3rd would pass. going to push my latest commit now.

- use heap.fixedBufferAllocator with size structInfo.fields.len * 512 rather then user allocator - get rid of tests expecting error.AllocatorRequired - add test "many field aliases" - make FieldAlias struct pub and add some doc comments

daurnimator · 2021-06-09T13:47:39Z

lib/std/json.zig

+// build a map of aliases from field_name => alias_name
+fn buildFieldAliasMap(comptime T: type, options: ParseOptions, allocator: *mem.Allocator) !std.StringHashMapUnmanaged([]const u8) {
+    var result = std.StringHashMapUnmanaged([]const u8){};
+    if (options.field_aliases.len == 0) return result;


no need for this line I think?

daurnimator · 2021-06-09T13:49:57Z

lib/std/json.zig

@@ -1623,6 +1710,9 @@ fn parseInternal(comptime T: type, token: Token, tokens: *TokenStream, options:
                    }
                }
            }
+            var buf: [structInfo.fields.len * (1 << 9)]u8 = undefined;


I don't think the limit of 512 is appropriate; I was more thinking something like iterating over the fields (with an inline for) and calculating the actual max size of the hash table possible (if there is e.g. 5 fields, then the hash table can't have more than 5 entries).

ok thats what i was thinking too initially. but i'm not sure how to do it. can you provide an example of how to initialize a hash table with only N entries?

i think i found a solution. will push a commit soon.

- allocates backing data on the stack - much smaller memory requirement (around 40 bytes per field) - removes FixedBufferAllocator - adds error.TooManyFieldAliases check

- 8 is ArrayHashMap's linear_scan_max. if there are more than 8 entries, an index is allocated and used. otherwise a get() does a linear scan and no allocations occur. - use FixedBufferAllocator with capacity 9 * field_names.len bytes. 9 is the lowest multiple which doesn't OOM. i tested this with up to 2048 struct fields.

travisstaloch · 2021-06-12T06:48:33Z

no idea why the freebsd build failed. i can't tell from the logs.

Vexu · 2021-06-12T09:22:54Z

Timed out, but it seems like all the relevant tests were run.

travisstaloch · 2021-06-24T19:54:45Z

is there anything left to do on this or is it just waiting in line? just wanted to ping @daurnimator as there has been no action in a couple weeks. note that the most recent freebsd ci job seems to have run all relevent tests but just timed out for some reason. also know that it shouldn't be a problem for me to rebase as happened previously.

andrewrk

Isn't the idea behind this to do the aliasing at compile-time instead of runtime?

is there anything left to do on this or is it just waiting in line?
* if a struct T passed to parse(T, ...) contains a pub const __tags of the form
shown below, these aliases will be used as the json keys instead of the
field name.

I don't see this in the code

travisstaloch · 2021-11-16T00:58:09Z

In response to @andrewrk questions:

Isn't the idea behind this to do the aliasing at compile-time instead of runtime?

I'm not sure if this is currently possible. In this PR's first commit i had created the field aliases as a list of from_field, to_field pairs at comptime. However if memory serves, this caused an OOM error.

The aliases are now stored as an ArrayHashMapUnmanaged which is created with stack memory. This was done in an attempt to avoid allocations.

I don't see this in the code

The field_aliases were moved from a __tags struct decl to ParseOptions.field_aliases as requested by @daurnimator i believe.

- this is a rework of ziglang#8987. rebasing that old pr proved difficult so it was easier to just start over. - this patch has the same api and tests as ziglang#8987 but uses an enums.EnumMap rather than StringArrayHashMap. this allows `field_alias_map` to be made entirely on the stack and requires no allocator (not even a FixedBufferAllocator which the previous used for re-indexing its map).

travisstaloch · 2021-11-21T02:33:50Z

Closing in favor of #10193

daurnimator previously requested changes Jun 4, 2021

View reviewed changes

lib/std/json.zig Outdated Show resolved Hide resolved

This was referenced Jun 4, 2021

std.json: support field aliases #8982

Closed

std.json: support field aliases #8993

Closed

travisstaloch added 2 commits June 7, 2021 21:17

add test "field alias escapes"

eb536da

travisstaloch force-pushed the json-field-aliases branch from 52377d5 to eb536da Compare June 8, 2021 04:51

daurnimator reviewed Jun 8, 2021

View reviewed changes

lib/std/json.zig Outdated Show resolved Hide resolved

change ParseOptions.field_aliases to a struct

3cbc06c

daurnimator added the standard library This issue involves writing Zig code for the standard library. label Jun 8, 2021

passing test "field alias escapes"

e50e9b4

- now using correct escape sequence and key - attempt to reformat and add a few comments to `inline for (structInfo.fields) |field, i|` loop body

travisstaloch added 2 commits June 8, 2021 04:45

revert changes to std.mem which are not necessary

4b9d69c

reverse buildAliasMap() keys/values

017016b

- now maps from field_name => alias_name - this unifies and improves searching for aliases. - this also fixes missing check for field.name equality. - add test coverage: false positive matches

daurnimator suggested changes Jun 9, 2021

View reviewed changes

travisstaloch force-pushed the json-field-aliases branch from 516aae8 to be039ac Compare June 9, 2021 12:18

stack allocate field_alias_map

9d5222c

- use heap.fixedBufferAllocator with size structInfo.fields.len * 512 rather then user allocator - get rid of tests expecting error.AllocatorRequired - add test "many field aliases" - make FieldAlias struct pub and add some doc comments

travisstaloch force-pushed the json-field-aliases branch from be039ac to 9d5222c Compare June 9, 2021 12:41

daurnimator reviewed Jun 9, 2021

View reviewed changes

use StringArrayHashMapUnmanaged

22049c7

- allocates backing data on the stack - much smaller memory requirement (around 40 bytes per field) - removes FixedBufferAllocator - adds error.TooManyFieldAliases check

travisstaloch force-pushed the json-field-aliases branch from 78086e5 to 22049c7 Compare June 11, 2021 00:35

travisstaloch requested a review from daurnimator June 11, 2021 20:18

Hadron67 mentioned this pull request Aug 25, 2021

allow declaration shadowing but disallow ambiguous identifier references #678

Closed

andrewrk requested changes Nov 16, 2021

View reviewed changes

travisstaloch mentioned this pull request Nov 21, 2021

std.json: support field aliases #10193

Closed

travisstaloch closed this Nov 21, 2021

r4gus mentioned this pull request Feb 18, 2023

Alter the behaviour for specific struct fields in a convenient way r4gus/zbor#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std.json: support field aliases #8987

std.json: support field aliases #8987

travisstaloch commented Jun 4, 2021 •

edited

Loading

travisstaloch commented Jun 4, 2021

travisstaloch commented Jun 4, 2021 •

edited

Loading

andrewrk commented Jun 4, 2021

daurnimator left a comment

travisstaloch commented Jun 4, 2021

andrewrk commented Jun 4, 2021 •

edited

Loading

travisstaloch commented Jun 4, 2021

travisstaloch commented Jun 8, 2021

daurnimator commented Jun 8, 2021

travisstaloch commented Jun 8, 2021 •

edited

Loading

daurnimator commented Jun 8, 2021

travisstaloch commented Jun 8, 2021 •

edited

Loading

daurnimator commented Jun 8, 2021 •

edited

Loading

travisstaloch commented Jun 8, 2021 •

edited

Loading

daurnimator commented Jun 8, 2021

daurnimator left a comment

daurnimator Jun 9, 2021

daurnimator Jun 9, 2021

travisstaloch Jun 9, 2021

travisstaloch commented Jun 9, 2021

travisstaloch commented Jun 9, 2021

travisstaloch commented Jun 9, 2021

daurnimator Jun 9, 2021

daurnimator Jun 9, 2021

travisstaloch Jun 9, 2021

travisstaloch Jun 10, 2021

travisstaloch commented Jun 12, 2021

Vexu commented Jun 12, 2021

travisstaloch commented Jun 24, 2021

andrewrk left a comment •

edited

Loading

travisstaloch commented Nov 16, 2021

travisstaloch commented Nov 21, 2021

std.json: support field aliases #8987

std.json: support field aliases #8987

Conversation

travisstaloch commented Jun 4, 2021 • edited Loading

travisstaloch commented Jun 4, 2021

travisstaloch commented Jun 4, 2021 • edited Loading

andrewrk commented Jun 4, 2021

daurnimator left a comment

Choose a reason for hiding this comment

travisstaloch commented Jun 4, 2021

andrewrk commented Jun 4, 2021 • edited Loading

travisstaloch commented Jun 4, 2021

travisstaloch commented Jun 8, 2021

daurnimator commented Jun 8, 2021

travisstaloch commented Jun 8, 2021 • edited Loading

daurnimator commented Jun 8, 2021

travisstaloch commented Jun 8, 2021 • edited Loading

daurnimator commented Jun 8, 2021 • edited Loading

travisstaloch commented Jun 8, 2021 • edited Loading

daurnimator commented Jun 8, 2021

daurnimator left a comment

Choose a reason for hiding this comment

daurnimator Jun 9, 2021

Choose a reason for hiding this comment

daurnimator Jun 9, 2021

Choose a reason for hiding this comment

travisstaloch Jun 9, 2021

Choose a reason for hiding this comment

travisstaloch commented Jun 9, 2021

travisstaloch commented Jun 9, 2021

travisstaloch commented Jun 9, 2021

daurnimator Jun 9, 2021

Choose a reason for hiding this comment

daurnimator Jun 9, 2021

Choose a reason for hiding this comment

travisstaloch Jun 9, 2021

Choose a reason for hiding this comment

travisstaloch Jun 10, 2021

Choose a reason for hiding this comment

travisstaloch commented Jun 12, 2021

Vexu commented Jun 12, 2021

travisstaloch commented Jun 24, 2021

andrewrk left a comment • edited Loading

Choose a reason for hiding this comment

travisstaloch commented Nov 16, 2021

travisstaloch commented Nov 21, 2021

travisstaloch commented Jun 4, 2021 •

edited

Loading

travisstaloch commented Jun 4, 2021 •

edited

Loading

andrewrk commented Jun 4, 2021 •

edited

Loading

travisstaloch commented Jun 8, 2021 •

edited

Loading

travisstaloch commented Jun 8, 2021 •

edited

Loading

daurnimator commented Jun 8, 2021 •

edited

Loading

travisstaloch commented Jun 8, 2021 •

edited

Loading

andrewrk left a comment •

edited

Loading