feat(rust): describe the option vocabulary as a JSON schema - #728
wkirschenmann wants to merge 4 commits into
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
| )] | ||
| #[serde(rename_all = "PascalCase")] | ||
| struct RawTls { | ||
| pub(crate) struct RawTls { |
There was a problem hiding this comment.
Why is RawTls required ? Can't we just use rawIdentity by providing deserialize and the tryfrom on Indentity ?
There was a problem hiding this comment.
Removable, but keeping it - and the macro is not the constraint. embed_prefixed! already takes the
value and schema types separately, and 3 of 5 units pass different ones, so TlsConfig, TlsConfig
would compile and change nothing.
RawTls is the document's shape, not a wrapper: all four fields differ in type from TlsConfig's
(String against bool, a loaded Option<Identity>, a loaded Option<CertificateDer>,
Option<String>). Dropping it moves each mapping onto TlsConfig as a deserialize_with plus a
schemars(with) - about eight attributes on a public #[non_exhaustive] type, each declaring that
a field is read as something other than its declared type. RawIdentity stays either way.
The rule already in the code: a Raw type exists exactly when those two shapes differ. tcp/http2
have none; tls maps text to a bool, to a file read into DER, and to an Option.
| identity: RawIdentity, | ||
| /// Path to the Certificate Authority file, in PEM format; empty for the system CAs. | ||
| #[serde(default, deserialize_with = "crate::config_utils::text")] | ||
| ca_cert: String, |
There was a problem hiding this comment.
This should be a named as a path
0efe361 to
72fd665
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72fd665f16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// [`RawIdentity::load`] is what reads empty as unset, whichever shape matched. | ||
| #[cfg(feature = "serde")] | ||
| #[derive(Debug, serde::Deserialize)] | ||
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] |
There was a problem hiding this comment.
When a document supplies only CertPem or only KeyPem, the generated schema accepts it through the Bare alternative because both fields have serde(default) and are therefore optional in that branch. Deserialization subsequently rejects the same document in RawIdentity::load with the both-or-neither error, so schema validators and generated consumers can produce configurations that the advertised reader refuses; model the empty alternative as requiring neither field while the PEM alternative requires both.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'll document why you're wrong.
72fd665 to
58a2abb
Compare
The options are becoming an FFI contract: a .NET consumer generates its own options type from them, and a hand-written mirror of the vocabulary would drift from the code the day someone forgot it. The `schema` feature derives the description from the types that already define the vocabulary, so there is one description and it cannot go stale. Nothing is committed: examples/generate_schema.rs prints the schema for whoever wants a file, and the tests pin the invariants a consumer builds against - every option under its flat name, the prefixed groups keeping their spellings, the identity alternatives as an anyOf, nothing promised that the reader ignores, and no Rust-typed default surviving anywhere - rather than a byte-for-byte rendering that a schemars upgrade would break without anything actually having been promised. Two mechanisms bridge what the derive cannot see on its own: a per-field `with = "String"` where the wire form is text and the Rust type is not, and a transform stripping defaults that serialise in the field's own type. The prefix needs a third, since `with_prefix!` rewrites keys where schemars cannot follow, and it folds into `embed_prefixed!` so that an embedding declares its prefix, its reader and its schema in one place and the three cannot drift apart. A unit read under a non-empty prefix stops spelling an option name in its field docs: those docs are the schema's descriptions, and the unprefixed spelling is not what any source writes. cargo test -p armonik-transport --all-features: 80 passed, 0 failed. cargo clippy -p armonik-transport --all-features --all-targets: 0 warnings. cargo clippy -p armonik --all-features --all-targets: 0 warnings. cargo check -p armonik-transport --no-default-features: 0 warnings. cargo fmt --check: clean. cargo run -p armonik-transport --features schema --example generate_schema: prints the schema.
A document naming only `CertPem` matches the `Bare` shape, so the generated schema accepts it, and `RawIdentity::load` then refuses it by name. That is a real gap between the two, and it is meant: the schema says which options exist and which arrive together, while a rule about two options at once is enforced once, where the message can name both. `dependentRequired` would express this one, but a rule written twice is a rule that drifts. The half a consumer needs sits on the `Bare` variant, because schemars keeps only variant docs for a flattened enum and drops the enum's own. The paragraph explaining the division stays in the source and says so, so the next person does not put it somewhere the schema will swallow. cargo test -p armonik-transport --all-features: 80 passed, 0 failed. generate_schema: the `Bare` description is in the emitted anyOf.
The option is a path to a PEM file, not the certificate, and every other file-valued option says so in its doc but not in its name. The field it is read from becomes `ca_cert_path`, so the flat name follows from the prefix mechanism with no rename attribute. `TlsConfig::ca_cert` keeps its name: it holds the loaded certificate, which is what the program has once the path has been read. Breaking, and it is a fourth deliberate divergence from ArmoniK's C# client, which spells the option `CaCert`. A deployment serving both clients names the file twice until the C# rename lands; the README says so, and the C# side is tracked separately. cargo test -p armonik-transport --all-features: 80 passed, 0 failed. cargo clippy -p armonik-transport --all-features --all-targets: clean. generate_schema: emits `CaCertPath`, and no `CaCert` remains anywhere.
58a2abb to
c565058
Compare
Renaming the option to `CaCertPath` leaves `GrpcClient__CaCert` naming no option in the Rust client, and an option it does not know is ignored rather than refused. The TLS secure and mTLS secure legs set `AllowUnsafeConnection=false`, so the run would verify the mock server's self-signed certificate against the system authorities and fail the whole Rust suite with a handshake error that names no option. Both spellings are exported because both are read: the C# client still spells it `CaCert`, and the divergence is tracked by #736.
|
1 participant
The flat option vocabulary is becoming an FFI contract: a .NET consumer will
generate its own options type from it. A hand-written mirror would drift from
the code the day someone forgot it, so this derives the description from the
types that already define the vocabulary.
A
schemafeature (schemars 1.x, off by default) makesHttpConfigand theunits it embeds describe themselves. One schema, flat, exactly the names
deserialisation reads.
What is in it
schemafeature onarmonik-transport, pulling inschemarsandserde_json. It impliesserde, and nothing else changes when it is off.examples/generate_schema.rsprints the schema, for whoever wants a file:cargo run -p armonik-transport --features schema --example generate_schema.embed_prefixed!, so an embedding declaresits prefix, its reader and its schema in one place.
serde_with::with_prefix!has no schemars integration, hence a helper that reapplies the prefix to the
generated properties,
requiredkeys included, inside union branches too.strip_defaults, because schemars serialises a field'sDefaultin thefield's own type:
false, or aDurationas an object, on an option whoseschema type is string. Each option states its default in prose instead.
a schemars upgrade that moves a keyword around breaks nothing that was
actually promised.
Two deliberate choices
No mirror structs. The real types derive
JsonSchema;TlsConfigdescribesitself through
RawTls, the shape it already deserialises from, since a unitbuilt through
TryFromdescribes the shape a document writes rather than theshape the program keeps.
The schema promises only what the reader reads. The proxy is
#[serde(skip)]and no option feeds it, so no proxy option appears; a testpins that, because an option the schema declares and the reader ignores is a
generated field that silently does nothing.
A unit read under a non-empty prefix also stops spelling an option name in its
field docs. Those docs are now the schema's descriptions, and
Keepaliveisnot a name any source writes -
TcpKeepaliveis, and the prefix belongs to theembedding. Units under no prefix keep their names, where the two coincide.
Tests
6 new tests: 5 schema smoke tests, and one in
config_utilscovering theprefix helper's union-branch case, which no embedding exercises with a
non-empty prefix yet.
Gates