mlevkov commented on PR #3803:
URL: https://github.com/apache/iggy/pull/3803#issuecomment-5228210251
Thanks, both wording findings were correct and the second one was
load-bearing rather than cosmetic. Pushed `3cd136fe3`.
**`Deserialize` vs re-serialize.** You are right, and my sentence would have
led an author to drop the wrong derive. `sdk/src/{sink,source}.rs` do
`serde_json::from_str::<C>` under a `DeserializeOwned` bound, so the plugin's
struct is exactly what gets deserialized into. The guidance now says derive
`Deserialize` but not `Serialize`, and names the property that actually carries
the argument: nothing ever re-serializes the struct. Also corrected that
`plugin_config` reaches the runtime as TOML, as JSON posted to the control API,
or from env.
**The inventory.** Scoped to plugin-side callers, and it now says explicitly
that the others are not all mistakes, naming `HttpConfig::api_key` and the
`core/common` login / create-user / change-password / PAT payloads where the
credential is the payload by design. Framing those as oversights was the worse
half of that sentence.
**On the paired `deserialize_with`:** I have left it as a documented
warning, strengthened to name the failure directly rather than implying it, and
I would rather add the mechanism with its first consumer. Reasoning, in case
you disagree:
A `deserialize_with` guard is itself opt-in, so it does not close the hole
mechanically. An author who writes `serialize_with = serialize_redacted` and
forgets the matching `deserialize_with` is precisely the case it claims to
cover, and they get the silent placeholder-as-secret anyway. The shape that
cannot be half-applied is a newtype owning both directions, and I would rather
design that against a real call site than guess at one. This PR already adds
two helpers with no consumers; a third feels like the wrong direction until
something needs it.
Happy to add either form now if you would rather have it mechanical.
**On the `REDACTED` const sweep** (`api/config.rs` Debug impl, core/common
auth credentials, server state models, s3_sink URL redaction): agreed, and I
read your note as out of scope here. Say the word and I will open it as its own
PR rather than widen this one.
Gate: fmt, sort, clippy `-D warnings`, `cargo test -p iggy_common
serde_secret` (6 pass), taplo, hawkeye, typos, markdownlint all exit 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
