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.

Sign up for GitHub

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

improve crashtracker config deserialization #588

Closed
wants to merge 2 commits into from
Closed

improve crashtracker config deserialization #588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c0db69
improve crashtracker config deserialization
rochdev Aug 15, 2024
abdd41e
fix lint
rochdev Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crashtracker/src/crash_info/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use ddcommon::tag::Tag;
use serde::{Deserialize, Serialize};

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)]
pub struct CrashtrackerMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we would want this to have empty defaults. Any reason to allow this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not if there are already defaults outside of Serde and they work 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that you would want the user to fill some of these fields (like family). Feel free to ignore I'm not sure how you would make that field mandatory.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the main thing. I think a configuration object struct should accept basically anything, and it should be up to the implementation using it to validate if the fields have values that are valid for whatever class using the object. In that sense, I think having a default value for every field makes sense, and simplifies usage by language-specific bindings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, in this case the default value is not valid and we should make it invalid by construction rather than validation.
For instance family really should be an enum, although right now it's more practical for it to be a String.
People should think about what they want in their metadata and deriving Default automatically goes against that

Copy link
Member Author

Choose a reason for hiding this comment

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

I was replying basically for all 3 objects, but for CrashtrackerMetadata I tend to agree that most fields are mandatory, I added Default basically because I was doing it also for the other ones. I'll change that one so that only tags are optional.

pub library_name: String,
pub library_version: String,
Expand Down
9 changes: 6 additions & 3 deletions crashtracker/src/shared/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,18 @@ use serde::{Deserialize, Serialize};
/// We recommend fully enabling stacktrace collection, but having an environment
/// variable to allow downgrading the collector.
#[repr(C)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub enum StacktraceCollection {
/// Stacktrace collection occurs in the
#[default]
r1viollet marked this conversation as resolved.
Show resolved Hide resolved
Disabled,
WithoutSymbols,
EnabledWithInprocessSymbols,
EnabledWithSymbolsInReceiver,
}

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)]
#[serde(default)]
pub struct CrashtrackerConfiguration {
// Paths to any additional files to track, if any
pub additional_files: Vec<String>,
Expand All @@ -28,7 +30,8 @@ pub struct CrashtrackerConfiguration {
pub wait_for_receiver: bool,
}

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)]
#[serde(default)]
pub struct CrashtrackerReceiverConfig {
pub args: Vec<String>,
pub env: Vec<(String, String)>,
Expand Down
34 changes: 22 additions & 12 deletions ddcommon/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ struct SerializedUri<'a> {
path_and_query: Option<Cow<'a, str>>,
}

#[derive(Serialize, Deserialize)]
#[serde(untagged)]
enum StringOrSerializedUri<'a> {
String(String),
SerializedUri(SerializedUri<'a>),
}
Comment on lines +67 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a reason why this is serialized like that and not just as a String (which it used to in the past).

The reason is that non http uris (unix sockets unix:// , windows pipes, or file uris) are not correctly deserialized by the hyper::Uri from string implementation.

Could you add unit tests to check what happens when you try to deserialize non http Uris?
I'm interested in particular in paths starting with /, ./ and containing spaces as they have caused problems

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this is passed to Hyper directly, I assumed it was just a shortcut to serialize Url which is not serializable by default. If there is a good reason to pass a more complex object, I'm fine with also passing that structure instead of relying on a string and removing this change. However, I tend to prefer handling this kind of common use cases internally behind the API instead of externally, thus exposing Hyper limitations to the caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, how do I even get the tests to pass? Whatever I do it seems there are a lot of tests that are failing before I even touch anything, and I'm unable to make them pass.


fn serialize_uri<S>(uri: &hyper::Uri, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
Expand All @@ -82,19 +89,22 @@ fn deserialize_uri<'de, D>(deserializer: D) -> Result<hyper::Uri, D::Error>
where
D: Deserializer<'de>,
{
let uri = SerializedUri::deserialize(deserializer)?;
let mut builder = hyper::Uri::builder();
if let Some(v) = uri.authority {
builder = builder.authority(v.deref());
}
if let Some(v) = uri.scheme {
builder = builder.scheme(v.deref());
}
if let Some(v) = uri.path_and_query {
builder = builder.path_and_query(v.deref());
match StringOrSerializedUri::deserialize(deserializer)? {
StringOrSerializedUri::String(str) => str.parse().map_err(Error::custom),
Copy link
Contributor

Choose a reason for hiding this comment

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

are there ways to add a message here to differentiate between the errors ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the original error message will be properly remapped onto the new custom error, or at least that is my understanding of what this is doing"

StringOrSerializedUri::SerializedUri(uri) => {
let mut builder = hyper::Uri::builder();
if let Some(v) = uri.authority {
builder = builder.authority(v.deref());
}
if let Some(v) = uri.scheme {
builder = builder.scheme(v.deref());
}
if let Some(v) = uri.path_and_query {
builder = builder.path_and_query(v.deref());
}
builder.build().map_err(Error::custom)
}
}

builder.build().map_err(Error::custom)
}

/// TODO: we should properly handle malformed urls
Expand Down