foxglove / ws-protocol

Foxglove WebSocket protocol specification and libraries
MIT License
99 stars 60 forks source link

C++ type definition doesn't contain schemaEncoding field #875

Open paulsohn opened 1 week ago

paulsohn commented 1 week ago

Description

https://github.com/foxglove/ws-protocol/blob/a28e0b9c9c319a1803bc00184d76fdb97714e921/cpp/foxglove-websocket/include/foxglove/websocket/common.hpp#L77-L83

Tried to use client publish capability, but currently C++ type definition doesn't contain schemaEncoding field.

Steps To Reproduce

N/A

Expected Behavior should contain schemaEncoding in the definition, and adjust server behavior if needed

linear[bot] commented 1 week ago

FG-9447 C++

paulsohn commented 1 week ago

Sorry for misplaced ticket title.

jtbandes commented 1 week ago

Additionally it seems schema is never populated when received by the server... https://github.com/foxglove/ws-protocol/blob/a28e0b9c9c319a1803bc00184d76fdb97714e921/cpp/foxglove-websocket/include/foxglove/websocket/websocket_server.hpp#L1337-L1341

And is not included in the message sent from the client: https://github.com/foxglove/ws-protocol/blob/a28e0b9c9c319a1803bc00184d76fdb97714e921/cpp/foxglove-websocket/include/foxglove/websocket/websocket_client.hpp#L22-L25

paulsohn commented 1 week ago

@jtbandes

If it's just unimplemented feature, then schema might take some breaking change as well like

struct ClientAdvertisement {
  // ...omit
  std::string schemaName;
  std::optional<std::vector<uint8_t>> schema; // or even std::optional<std::string> schema;
  std::optional<std::string> schemaEncoding;
};

I think std::string is appropriate compared to std::vector<uint8_t> because schemas are advertised as json field.