Mark unrelated parsing error as warning

[photovoltex]

The parsing error was multiple times falsely refereed to as possible cause, even tho it is completely unrelated to the main issue in most cases.

The issue will always happens when playing from an official player and then transferring the playback to librespot. The log message looked like the following:

[2025-04-20T09:14:08Z ERROR librespot_connect::spirc] could not parse session_update: Invalid state { Unknown enum variant name: WIFI_BROADCAST_CHANGED at 1:11 }

This change reduces the log level for the parsing failure for session_update to warn, while adding an additional message to highlight why it is only a warning.

[Copilot]

Pull Request Overview

This PR updates the logging behavior for parsing errors, ensuring that unrelated errors (e.g. WIFI_BROADCAST_CHANGED) are marked as warnings rather than errors.

• Modified the unwrap! macro to accept a logging macro and a custom message.
• Added a fallback branch for legacy macro usage.
• Changed the logging level for session_update parsing errors to warn.

[roderickvd]

Why not just ignore the known parsing error entirely, or just exposing it with trace verbosity?

[photovoltex]

Why not just ignore the known parsing error entirely

I don't want to ignore an parsing error, because in the end it is an error that should be addressed, and I only wanted to mark it as "it's not anything breaking" for now.

trace verbosity

And putting in on trace verbosity would also hide the error which might result in ignoring it until it becomes an actual problem.

I will take a look at it again, how we could handle it maybe more graceful. I would like to keep the protobuf files as close to its original but to fix the core issue we would need to adjust those.

[roderickvd]

I will take a look at it again, how we could handle it maybe more graceful. I would like to keep the protobuf files as close to its original but to fix the core issue we would need to adjust those.

I agree the protobufs should be kept as they are. Major foot shooting looms otherwise.

[photovoltex]

So I tried some stuff today.

Initially I just modified the protobuf-json-mapping crate slightly to allow a fallback when a enum hadn't any match. You could modify the settings for the parser and that worked like a charm. That solution tho would require the whole open source contribution cycle, which would take some time (not that this would be an issue, but still something to consider).

Because of that I did go on a little journey and tried prost with some hopes of having better control over the generated code and maybe being able to switching to native serde serialization. The whole refactor did take some time but it worked in the end enough to test if it would improve the here mentioned problem case.

And as it turns out prost does handling and serializing of enums quite a bit differently.

The enum type isn't used directly as a field, because the Protobuf spec mandates that enumerations values are 'open', and decoding unrecognized enumeration values must be possible.

The generated code for the SessionUpdate would look like this:

#[derive(serde::Serialize, serde::Deserialize)]
#[serde(rename_all = "snake_case")]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct SessionUpdate {
    #[prost(message, optional, tag = "1")]
    pub session: ::core::option::Option<Session>,
    #[prost(enumeration = "SessionUpdateReason", tag = "2")]
    pub reason: i32,
    #[prost(message, repeated, tag = "3")]
    pub updated_session_members: ::prost::alloc::vec::Vec<SessionMember>,
}

Notice that the reason is a i32 instead of the enum SessionUpdateReason

That little detail tho means the struct itself doesn't know which enum it would need to use when serializing from json and by that the whole journey with prost seems to end here. You could probably just use a custom de/serialize_with but that again would be necessary for each enum, or just work for some enums, which seems quite restrictive.

---

So in the end, I think if we want to release a new version soon, we should settle on a quick solution rather then a complete fix or big refactor. I would be fine with reducing the issue to trace and marking it in code as todo and come back to it later with a potential new option from the protobuf crate or a different solution with prost (as they technically have json support on their roadmap).

A short feedback on the matter would be appreciated :)

I will do the trace adjustment in the following days if no other concern's are raised.

[roderickvd]

To keep it short then: sounds good to me 😄

[photovoltex]

So, I adjusted the handling a bit more then I initially planed. But with that it should be better represented that the parsing from a json string isn't as smooth as it is when parsing from bytes.

[Copilot]

Pull Request Overview

This PR adjusts the log level for parsing errors and modifies the session update handling to better indicate when a parsing failure is not a critical issue. Key changes include:

• Removing extra logging in the deserialize_with module.
• Introducing a FallbackWrapper enum and updating JSON deserialization in the protocol.
• Changing the log level for missing subscribers and adjusting session update handling in spirc.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
core/src/deserialize_with.rs	Removed extra warning/error log messages to simplify error conversion.
core/src/dealer/protocol.rs	Added FallbackWrapper and updated JSON deserialization to use try_from_json.
core/src/dealer/mod.rs	Lowered log level from warn to debug for missing subscriber notifications.
connect/src/spirc.rs	Updated session_update handling to use FallbackWrapper and refined error logging.

Comments suppressed due to low confidence (1)

core/src/dealer/mod.rs:360

• [nitpick] Verify that reducing the log level to debug for missing subscribers does not reduce necessary visibility in production scenarios where this information might be critical for troubleshooting.

debug!("No subscriber for msg.uri: {}", msg.uri);

[roderickvd]

Pretty elegant!

[kingosticks]

Yeh, nice!