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

Add API for deserializing data envelopes from JSON #395

Merged
merged 2 commits into from
May 9, 2024
Merged

Conversation

Neverlord
Copy link
Member

Closes #391.

@Neverlord Neverlord marked this pull request as ready for review March 17, 2024 12:57
@Neverlord Neverlord marked this pull request as draft March 17, 2024 14:54
@Neverlord
Copy link
Member Author

Reverting back to draft, it doesn't deserialize the topic from the data yet.

@Neverlord Neverlord marked this pull request as ready for review March 17, 2024 18:23
@Neverlord
Copy link
Member Author

Ready to go now.

return error{ec::invalid_json};
auto obj = val->to_object();
// Type-checking.
if (obj.value("type").to_string() != "data-message")
Copy link
Contributor

@awelzel awelzel Mar 18, 2024

Choose a reason for hiding this comment

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

Hmm, I realize I'm looking at parsing one layer further in: The idea was JSON contains only the serialized event vector without the data-message and topic.

In my use-case, "type" and "topic" are in message headers, not within the payload and the payload isn't meant to be structured like the websocket protocol.

Let me check if the JSON serialization in #391 even does what I thought it did.

Copy link
Member Author

Choose a reason for hiding this comment

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

So you'd basically want to have a function that parses you just the variant from the JSON format? I'll add that later.

Copy link
Contributor

@awelzel awelzel Mar 18, 2024

Choose a reason for hiding this comment

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

So, the following snippet:

        std::vector<char> cbuf;
        auto ev = broker::zeek::Event(event.HandlerName(), std::move(xs), broker::to_timestamp(event.timestamp));
        broker::format::json::v1::encode(ev.move_data(), std::back_inserter(cbuf));

Produces JSON as follows (where the object encoding a Zeek event is at the top-level), similarly how this works for the binary protocol shown in #391. There's no topic or type, just the "event vector directly".

[#1] Received on "zeek.cluster.discovery"
{"@data-type":"vector","data":[{"@data-type":"count","data":1},{"@data-type":"count","data":1},{"@data-type":"vector","data":[{"@data-type":"string","data":"Cluster::Backend::NATS::hello"},{"@data-type":"vector","data":[{"@data-type":"string","data":"proxy-1"},{"@data-type":"string","data":"nats_tinkyx1_2491709_NCcEAFMz82U3"}]},{"@data-type":"vector","data":[{"@data-type":"vector","data":[{"@data-type":"count","data":1},{"@data-type":"timestamp","data":"1970-01-01T01:00:00.000"}]}]}]}]}

And that is what I was hoping to parse. For the binary protocol that can be achieved via the following:

    auto r = broker::data_envelope::deserialize(broker::endpoint_id::nil(), broker::endpoint_id::nil(), 0, "", payload,
                                                payload_size);
    broker::zeek::Event ev(std::move(*r));

Am I looking for broker::data_envelop::deserialize_json(...) instead? Maybe that doesn't make overly much sense.

Now, the main motivation is to "just" re-use existing encoding/serialization functionality from broker for events. If you think that API doesn't make much sense to have, feel free to push back. On the other hand, there is an API for serialization, so it might make sense to offer one for de-serialization as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, the main motivation is to "just" re-use existing encoding/serialization functionality from broker for events. If you think that API doesn't make much sense to have, feel free to push back.

No push back at all, I just didn't think of that use case yet. 🙂

I'll add functions that do the JSON (de)-serialization for variant as well.

@Neverlord
Copy link
Member Author

All done.

@timwoj
Copy link
Member

timwoj commented May 6, 2024

I reviewed this a while ago, and just realized that I never approved it.

@Neverlord Neverlord merged commit 04ee6fd into master May 9, 2024
20 checks passed
@Neverlord Neverlord deleted the issue/gh-391 branch May 9, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make broker::data_envelope::deserialize() for JSON format accessible
3 participants