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

Parsing bytes from reader can lead to expected a borrowed string error #55

Closed
mdibaiee opened this issue Jun 28, 2022 · 2 comments · Fixed by #71
Closed

Parsing bytes from reader can lead to expected a borrowed string error #55

mdibaiee opened this issue Jun 28, 2022 · 2 comments · Fixed by #71
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@mdibaiee
Copy link

mdibaiee commented Jun 28, 2022

Hello!

We are using pbjson on our prost messages that look like this:

https://github.com/estuary/flow/blob/master/crates/proto-flow/src/capture.rs#L191-L192

When I try to deserialize a JSON string that looks like this (all other fields omitted for brevity)

{
  "open": {
    "driverCheckpoint": "e30=",
  }
}

I'm getting this error from the deserializer:

`Err` value: invalid type: string "e30=", expected a borrowed string at line 25 column 30'

My understanding is that this is because of this line: https://github.com/influxdata/pbjson/blob/main/pbjson/src/lib.rs#L67

After searching a bit, I found these:

My understanding is that I'm getting this error because I'm deserializing from a Reader, namely stdin:

let request: PullRequest = serde_json::from_reader(stdin)?;

This means that the underlying data for deserialization (the bytes from stdin) are destroyed as soon as they are read, therefore a reference to this data will not be valid once the data is dropped. In this case, we would have to deserialize to an owned String.

The workaround at the moment is to read the whole value from Reader, and then deserialize:

let mut buf = Vec::new();
stdin.read_to_end(&mut buf)?;
let request: PullRequest = serde_json::from_slice(&buf)?;
@tustvold tustvold added the bug Something isn't working label Jun 29, 2022
@tustvold
Copy link
Contributor

Thank you for the report, I'm a little bit swamped at the moment with other projects so I'm not sure when I'll have a chance to get to this, but if you, or anyone else, felt able to propose a fix I would be more than happy to give it a review 😄

@tustvold tustvold added the help wanted Extra attention is needed label Jun 29, 2022
vthib added a commit to JustRustThings/pbjson that referenced this issue Sep 27, 2022
The pattern "let s: &str = Deserialize::deserialize(deser)" used
in multiple places is not ideal, as it generates errors when the
deserializer uses an IO Reader and not a string. To fix this,
implementing a visitor is preferred, as it gives the deserializer the
choice of allocating or not a string.

To ensure the issue is fixed, the tests now deserialize both from a
string and from a reader.

Closes influxdata#55
@vthib
Copy link
Contributor

vthib commented Sep 27, 2022

I have stumbled upon this issue as well, when deserializing from an opened file. I have pushed a PR to fix this, as well as other issues that arose when I added the serde_json::from_reader check on all the kitchen sink tests: #71

@kodiakhq kodiakhq bot closed this as completed in #71 Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants