Skip to content

Fix json oneof shadowing#163

Merged
Arwalk merged 5 commits into
Arwalk:masterfrom
inge4pres:issues/147
Mar 9, 2026
Merged

Fix json oneof shadowing#163
Arwalk merged 5 commits into
Arwalk:masterfrom
inge4pres:issues/147

Conversation

@inge4pres

Copy link
Copy Markdown
Contributor

Reason for this PR

Closes #147
Supersedes: #148

We want to give users ability to discard the nested container value in JSON encoding/decoding.

Note for reviewers: don't get scared by the large diff, reviewing by commit should be doable. The second commit has an update to the exiting API that cascades to all generated files.
Let me know if you prefer to keep the second commit or if we should split the API adding a "WithOptions" public variant.

Details

Split by commit:

  1. Add an Options struct to configure the encoding/decoding behavior
  2. Change the json.stringify API to add an optional parameter passing user-provided Options (mouthful not intended 😅)

Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>

@Arwalk Arwalk left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woudln't it be possible to have the options exposed in the protobuf api?

i think that we could have something like:

generated file:

pub fn jsonEncode(
        self: @This(),
        stdJsonOptions: std.json.Stringify.Options,
        protobufJsonOptions : protobuf.json.Options,
        allocator: std.mem.Allocator,
    ) ![]const u8 {
        return protobuf.json.encode(self, stdJsonOptions, protobufJsonOptions, allocator);
    }

json.zig:

pub fn encode(
    data: anytype,
    stdJsonOptions: std.json.Stringify.Options,
    protobufJsonOptions : protobuf.json.Options,
    allocator: std.mem.Allocator,
) ![]u8 {
    const Wrapper = struct {
        data : @TypeOf(data),
        pbJsonOptions : protobuf.json.Options,

         pub fn jsonStringify (self: *const @This(), jws: anytype) !void {
            return protobuf.json.stringify(@This(), self, jws, self.pbJsonOptions);
         }
    }
    
    return std.json.Stringify.valueAlloc(allocator, Wrapper{.data = data, .pbJsonOptions = pbJsonOptions},  stdJsonOptions);
}

pub fn stringify(Self: type, self: *const Self, jws: anytype, opts: ?Options) !void {
   const data = self.data;
   const  pbJsonOptions = self.pbJsonOptions;
   // work with that 
}

This might even allow us to remove the jsonStringify from the generated structs.
I'm not sure if this works, this is just a theory. What do you think @inge4pres ?

@inge4pres

inge4pres commented Feb 26, 2026

Copy link
Copy Markdown
Contributor Author

@Arwalk that seems reasonable, and I think it's a great idea! Because right now using encode is the right way to get a JSON, and stringify is a specialization of that.
It comes with a change in public-facing API but I think that's tolerable as long as the minor version is increased.

I noticed there is additional logic that there is in stringify compared to encode. I think we should keep that logic (which does camel_case conversion and with this PR oneOf removal) and move it inside encode if we want to remove the duplication, but that comes with a breaking change correct?

@Arwalk

Arwalk commented Feb 26, 2026

Copy link
Copy Markdown
Owner

I'm ok with breaking changes in the JSON interfaces. As stated in the README, it's still considered beta. Still, we'll increase the major version accordingly (as it will be a non-retro-compatible interface change and we're doing our best to stay true to semantic versioning)

Thanks again @inge4pres !

@inge4pres

inge4pres commented Feb 28, 2026

Copy link
Copy Markdown
Contributor Author

I'm ok with breaking changes in the JSON interfaces.

@Arwalk nice thanks, LGTM I will proceed if you can confirm my assumption in the previous comment

I think we should keep that logic (which does camel_case conversion and with this PR oneOf removal) and move it inside encode if we want to remove the duplication, but that comes with a breaking change correct?

@Arwalk

Arwalk commented Mar 2, 2026

Copy link
Copy Markdown
Owner

I think we should keep that logic (which does camel_case conversion and with this PR oneOf removal) and move it inside encode if we want to remove the duplication, but that comes with a breaking change correct?

Sorry i wasn't clear on that. Yes, you can go ahead. Thanks again for everything.

This is a breaking change!
json.encode has a new signature accepting decode options
and supersedes the existing json.stringify API.

Signed-off-by: inge4pres <fgualazzi@gmail.com>
Signed-off-by: inge4pres <fgualazzi@gmail.com>
@inge4pres

Copy link
Copy Markdown
Contributor Author

It's done! 🚀
I split in 2 more commits the changes described above.

This is a breaking change so mind it when merging, a new major version upgrade will be needed.

@inge4pres inge4pres requested a review from Arwalk March 3, 2026 20:54

@Arwalk Arwalk left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes on the tests but overall this is a great job.

Comment thread tests/tests_json.zig Outdated
Comment thread tests/tests_json.zig Outdated
Comment thread tests/tests_json.zig Outdated
Signed-off-by: inge4pres <fgualazzi@gmail.com>
@inge4pres

Copy link
Copy Markdown
Contributor Author

Minor changes

Feedback incorporated 🚀

@inge4pres inge4pres requested a review from Arwalk March 6, 2026 22:25
@Arwalk Arwalk merged commit 622e9e8 into Arwalk:master Mar 9, 2026
5 checks passed
@Arwalk

Arwalk commented Mar 9, 2026

Copy link
Copy Markdown
Owner

Really cool work. I really like how passing custom options is handled here. I hope i'll find the time to make a blog post or something for other people that'd like to have custom informations in their json encoding.

Thanks @inge4pres , i'll do a release soonish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect JSON serialization for otel protos

2 participants