Skip to content

Change ClientChannel schema data to binary; unify base64 handling #285

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

Merged
merged 3 commits into from
Mar 14, 2025

Conversation

jtbandes
Copy link
Member

Changelog

None

Docs

None

Description

The current websocket protocol uses JSON messages for advertisements (in both directions), which requires binary schemas to be base64-encoded. Currently the SDK handles base64-encoding when sending protobuf schemas to the client (although we missed also doing this for flatbuffer schemas), but ClientAdvertisements left the base64 handling up to the user (they would just receive a String which may or may not be base64-encoded). This PR adds base64 handling for the client advertisements as well so the SDK handles both directions and doesn't require the user to mess with base64.

  • Changed ClientChannel.schema to Option<Vec<u8>>
  • Added JsonClientChannel & JsonClientAdvertise types for deserialization, and ClientChannel::try_from(JsonClientChannel) to do the base64 decoding
  • Treat flatbuffer as base64 as well as protobuf (docs)

@jtbandes jtbandes requested review from eloff, bryfox and gasmith March 14, 2025 00:07
Copy link
Contributor

@eloff eloff left a comment

Choose a reason for hiding this comment

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

This looks good. It's nicer to take care of the base64 decoding here so the user doesn't have to do that.

@jtbandes jtbandes merged commit 1fda013 into main Mar 14, 2025
38 checks passed
@jtbandes jtbandes deleted the jacob/client-channel branch March 14, 2025 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants