Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CdpEvent::InvalidParams variant #207

Closed
wants to merge 3 commits into from

Conversation

ryo33
Copy link
Contributor

@ryo33 ryo33 commented Feb 15, 2024

Fixes #167

Add CdpEvent::InvalidParams(serde_json::Value), and use it if deserializing an event param for a specific method fails.
There already are CdpEvent::Other(serde_json::Value), but it's only used for unknown method ids.

I use the raw_value feature on serde_json to get serde_json::Value without any allocation after deserializing an event param failed.

ureq = "2.6.2"
tempfile = "3.2.0"

[dependencies]
chromiumoxide_pdl = { path = "../chromiumoxide_pdl", version = "0.5"}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this line because it seems unnecessary.

#deserialize_from_method
_=>CdpEvent::Other(map.next_value::<serde_json::Value>()?)
_ => CdpEvent::Other(serde_json::from_str::<serde_json::Value>(value.get()).unwrap())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These unwrap() should never panic, because RawValue can only be deserialized from a valid JSON, and Value can be serialized from any valid JSON.

@tgrushka
Copy link
Contributor

Hey @ryo33 and @mattsse , any idea when this might be merged? I'm hoping it might fix CdpError::Serde errors like in the following (?):

    let handle = tokio::task::spawn(async move {
        while let Some(h) = handler.next().await {
            if let Err(CdpError::Serde(err)) = h {
                println!("Serde error: {:#?}", err);
                continue;
            } else if let Err(err) = h {
                println!("CDP Error: {:#?}", err);
                break;
            }
        }
    });

Meanwhile, I guess it's safe to just ignore these errors as those CDP messages are currently unsupported anyway... probably just new Chrome/ium experimental features?

@Sytten
Copy link
Contributor

Sytten commented Oct 24, 2024

This is not the solution because we try to translate that to a custom event which means the user would need to have a listener with a custom event for each method, this is not viable.
The real solution is really to let it fail if the user asks for that or ignore the event entirely.
@mattsse this can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Critical Deserialization issue
3 participants