Add opt-in protobuf-JSON serde support to Rust binding#279
Conversation
Add a `serde` cargo feature that generates protobuf-JSON Serialize/Deserialize impls via pbjson, scoped to PersistentStorageSettings. The generated code matches C++ MessageToJsonString conventions for the settings file: snake_case proto field names, default-valued fields always emitted, and unknown fields ignored on read (accepting both snake_case and camelCase keys). Generation is deliberately limited to PersistentStorageSettings: other messages need the opposite emit-defaults behavior, and package-wide generation would cover well-known types that prost-types has no serde impls for. The feature is off by default; the no-feature build output is byte-identical to before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an opt-in serde Cargo feature to the Rust blueye_protocol crate to generate protobuf-JSON Serialize/Deserialize implementations (via pbjson) for PersistentStorageSettings, aligning JSON output with MessageToJsonString-style conventions and reducing the risk of fields silently being omitted in persisted settings.
Changes:
- Introduces a
serdefeature with optionalpbjson/serdedeps, pluspbjson-buildfor codegen andserde_jsonfor tests. - Extends
build.rsto generate a feature-gatedblueye.protocol.serde.rswith pbjson-build configured for snake_case, emit-defaults, and ignore-unknown-fields. - Adds feature-gated Rust tests validating JSON key casing, emitting default-valued fields, unknown-field tolerance, and round-trip behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/Cargo.toml | Adds the opt-in serde feature and declares the new (optional) serde/pbjson dependencies plus test dependency. |
| rust/build.rs | Conditionally runs pbjson-build codegen when CARGO_FEATURE_SERDE is enabled. |
| rust/src/lib.rs | Feature-gated inclusion of the generated pbjson serde impls alongside the prost-generated bindings. |
| rust/tests/serde_json.rs | Adds feature-gated tests for protobuf-JSON serialization/deserialization conventions for PersistentStorageSettings. |
| rust/Cargo.lock | Locks new dependency resolutions introduced by pbjson/serde/serde_json additions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for key in expected_keys { | ||
| assert!(object.contains_key(key), "missing key {key:?} in {object:?}"); | ||
| } | ||
| assert_eq!(object.len(), expected_keys.len()); |
There was a problem hiding this comment.
Fixed in e69f1fa, with a stronger assertion instead of >=: the expected key set is now derived from the message descriptor via prost-reflect (already a dependency), and the fixture uses ..Default::default(). Relaxing to >= wouldn't have removed the churn anyway — the exhaustive struct literal in the fixture also broke compilation on every new field. Now new proto fields require no test changes, and the test automatically verifies each one is emitted, which is the point of generating the serializer.
| [dev-dependencies] | ||
| serde_json = "1.0" |
There was a problem hiding this comment.
Cargo doesn't support this: optional = true under [dev-dependencies] is rejected with "dev-dependencies are not allowed to be optional" (verified against this manifest), and cfg(feature = ...) isn't allowed in target-specific dependency tables either. Since dev-dependencies are never resolved by downstream consumers, the only cost is fetching serde_json when running cargo test in this repo, so leaving it unconditional.
The exact key-count assertion (and the exhaustive struct literal in the fixture) meant every new PersistentStorageSettings field would break this test. Deriving the expected key set from the prost-reflect descriptor and defaulting unset fixture fields keeps the test in sync with the schema automatically, while strengthening the assertion: every schema field must appear in the JSON output, including future ones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Adds an opt-in
serdecargo feature to the Rust crate that generates protobuf-JSONSerialize/Deserializeimpls via pbjson, scoped toPersistentStorageSettingsonly.The motivation is the drone's
/data/.persistent_storage_settings.jsonfile: the downstream consumer (libblunux) currently hand-writes the JSON serializer to match C++MessageToJsonStringconventions, so new proto fields can silently go missing from the file. Generating the serializer from the schema closes that gap.Changes
rust/Cargo.toml: newserdefeature pulling in optionalpbjson/serdedeps;pbjson-build0.9 in build-dependencies (requires prost ^0.14, matching the existing prost 0.14). Off by default.rust/build.rs: when the feature is enabled (CARGO_FEATURE_SERDE), runs pbjson-build on the same protox descriptor set, configured with:preserve_proto_field_names()— snake_case keys, matching the existing file formatemit_fields()— default-valued fields are always writtenignore_unknown_fields()— lenient deserializationrust/src/lib.rs: feature-gated include of the generated serde impls.rust/tests/serde_json.rs: feature-gated tests covering snake_case keys, all-fields-emitted (includingfalse), unknown-field tolerance, acceptance of bothwebserver_logandwebserverLogspellings, and a round-trip.Generation is deliberately scoped to
.blueye.protocol.PersistentStorageSettingsrather than the whole package: other messages (guest-port info) intentionally need the opposite emit-defaults behavior, and package-wide generation would also cover well-known types that prost-types has no serde impls for.Verification
cargo build/cargo testwithout the feature: unchanged, all tests pass; the generatedblueye.protocol.rsis byte-identical to master.cargo build --features serde/cargo test --features serde: all 11 tests pass.blueye.protocol.serde.rscontains impls forPersistentStorageSettingsand nothing else.🤖 Generated with Claude Code