Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Json Stringify option to not write out null optional fields #8908

Closed
wants to merge 2 commits into from

Conversation

hazeycode
Copy link
Sponsor Contributor

Example usage:

const std = @import("std");

const Foo = struct {
    not_optional: []const u8 = "required",

    bar: struct {
        optional: ?[]const u8 = null,
    } = .{},
};

const writer = std.io.getStdOut().writer();

test "#1 stringify default (existing) behaviour" {
    try std.json.stringify(Foo{}, .{}, writer);
    try writer.writeAll("\n");
}

test "#2 stringify ignore nulls" {
    const options: std.json.StringifyOptions = .{ .string = .{ .String = .{ .write_null_optional_fields = false } } };
    try std.json.stringify(Foo{}, options, writer);
    try writer.writeAll("\n");
}

test 1 output: {"not_optional":"required","bar":{"optional":null}}

test 2 output: {"not_optional":"required","bar":{}}

@g-w1
Copy link
Contributor

g-w1 commented May 26, 2021

Please add a test.

@hazeycode hazeycode changed the title Json Stringify option to not write out null optional feilds Json Stringify option to not write out null optional fields May 26, 2021
@marler8997
Copy link
Contributor

With this option, would the resulting JSON also be parsed correctly?

@hazeycode
Copy link
Sponsor Contributor Author

hazeycode commented May 26, 2021

With this option, would the resulting JSON also be parsed correctly?

Good question. I think as it stands std.json.parse would fail if an optional field isn't present. I'd need to double check. If this is the case though, it should be possible to fix by have it check against a comptime array of required fields for type and set missing optional fields to null

@marler8997
Copy link
Contributor

Good question. I think as it stands std.json.parse would fail if an optional field isn't present. I'd need to double check. If this is the case though, it should be possible to fix by have it check against a comptime array of required fields for type and set missing optional fields to null

You may also want to add this as an optional feature to the parser as well.

@daurnimator
Copy link
Collaborator

FWIW I was originally intending on supporting "missing" fields by doing a union with void.
It doesn't currently work, but should be easy enough to retrofit so that the following code works:

const std = @import("std");

const Foo = struct {
    not_optional: []const u8 = "required",

    bar: struct {
        optional: union (enum) {
          data: []const u8,
          missing: void,
        } = .{.missing = {}},
    } = .{},
};


test "union with void" {
    const writer = std.io.getStdOut().writer();
    try std.json.stringify(Foo{}, .{}, writer);
    try writer.writeAll("\n");
}

@hazeycode
Copy link
Sponsor Contributor Author

hazeycode commented May 27, 2021

Good question. I think as it stands std.json.parse would fail if an optional field isn't present. I'd need to double check. If this is the case though, it should be possible to fix by have it check against a comptime array of required fields for type and set missing optional fields to null

You may also want to add this as an optional feature to the parser as well.

I'm not sure that it should be an optional feature of the parser. I think the parser should always set optional fields to null on result if they are not present in the json stream. In the case where the user wants to treat this as an error, they can do so simply by checking if the field on the typed result is null

@tauoverpi
Copy link
Sponsor Contributor

The current with having to declare a default value of null is imo better than having the parser do it automatically as it's easier to control failure and the intent is clear when reading the struct fields alone. Adding a test for being able to consume that which stringify produces isn't too much of a burden.

@hazeycode hazeycode closed this May 27, 2021
@hazeycode hazeycode reopened this May 27, 2021
@hazeycode
Copy link
Sponsor Contributor Author

The current with having to declare a default value of null is imo better than having the parser do it automatically as it's easier to control failure and the intent is clear when reading the struct fields alone. Adding a test for being able to consume that which stringify produces isn't too much of a burden.

If a web service gives them missing fields, shouldn't std.json.parse just work even if the user forgot to set defaults on their zig struct? Or perhaps they even want to default to a value other than null. I'm not sure but it seems like relying on default values leads to bad user experience.

@tauoverpi
Copy link
Sponsor Contributor

If a web service gives them missing fields, shouldn't std.json.parse just work even if the user forgot to set defaults on their zig struct?

It would mean their specification of the object is wrong and likely code dealing with it needs to be updated aswell to correctly conform to the API. Being more lenient feels like it would cause more errors than it should. This also matches with how you initialize the object yourself with .{} syntax thus maps nicely to how one (at least I) thinks about struct values.

Or perhaps they even want to default to a value other than null.

This is what is done now with struct { x: ?u32 = 4 } where struct { x: ?u32 = null } communicates the intent quite well.

I'm not sure but it seems like relying on default values leads to bad user experience.

Having better error reporting from std.json itself using #8584 (and possibly the extension to modules) or something similar should improve the experience enough to not be a problem. Currently the debugging experience isn't so great as error sets don't return enough information to find the issue without testing your way forward; automatically setting missing fields feels like it's trying to solve the issue from the wrong direction.

@hazeycode
Copy link
Sponsor Contributor Author

hazeycode commented May 27, 2021

If a web service gives them missing fields, shouldn't std.json.parse just work even if the user forgot to set defaults on their zig struct?

It would mean their specification of the object is wrong and likely code dealing with it needs to be updated aswell to correctly conform to the API. Being more lenient feels like it would cause more errors than it should. This also matches with how you initialize the object yourself with .{} syntax thus maps nicely to how one (at least I) thinks about struct values.

Or perhaps they even want to default to a value other than null.

This is what is done now with struct { x: ?u32 = 4 } where struct { x: ?u32 = null } communicates the intent quite well.

I'm not sure but it seems like relying on default values leads to bad user experience.

Having better error reporting from std.json itself using #8584 (and possibly the extension to modules) or something similar should improve the experience enough to not be a problem. Currently the debugging experience isn't so great as error sets don't return enough information to find the issue without testing your way forward; automatically setting missing fields feels like it's trying to solve the issue from the wrong direction.

What happens currently vs what do you expect should happen if we

const MyStruct = struct { x: ?u32 = 4 };
const foo = std.json.parse(MyStruct, stream, .{});

for the stream "{}"
?

@tauoverpi
Copy link
Sponsor Contributor

const A = struct { x: ?u32 };
const B = struct { x: ?u32 = 4 };
...
// stream of {}
const a = try parse(A, stream, .{}); // fails as no value provided
const b = try parse(B, stream, .{}); // works b.x = 4
...
const a: A = .{}; // compile error as no value provided
const b: B = .{}; // works b.x = 4

This is what I would expect from the parser which is the current behaviour.

@hazeycode
Copy link
Sponsor Contributor Author

Cool. I think I can agree that default values should not be overwritten. I had had my mind set on that they would be.
I will add a test for parsing stringify output.

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label May 30, 2021
@hazeycode
Copy link
Sponsor Contributor Author

Further testing revealed these changes only work in the very simplest of case. Closing for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants