-
Notifications
You must be signed in to change notification settings - Fork 185
RUST-1405 Expose full server response in Error
#1474
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
Conversation
3d372dc
to
e4a03f1
Compare
src/error.rs
Outdated
#[source] | ||
pub(crate) source: Option<Box<Error>>, | ||
|
||
pub(crate) server_response: Option<Box<Document>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd lean towards storing and exposing this as a RawDocumentBuf
- this feels like a "lower level" API where the wire representation is more appropriate, and also it allows inspecting the bytes in case the server sends a response that doesn't fully parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - I updated this to be a raw document. I did leave the tracing representation to be a parsed document for readability in logs (https://github.com/mongodb/specifications/blob/master/source/logging/logging.md#representing-documents-in-log-messages) with a hex string of the byte vec as a fallback.
fn tracing_representation(&self) -> String { | ||
self.to_string() | ||
impl crate::error::Error { | ||
fn tracing_representation(&self, max_document_length: usize) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to feature-conditionally thread this through everywhere is a bit unpleasant; what would you think about attaching max_document_length
as a field of Error
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this approach as it'll accommodate any future additions of fields needing truncation for the conection and SDAM emitters, and it feels consistent with other fields for the command emitter - happy to make the change if you feel strongly in the other direction, though
6f0e91b
to
7f3a4c3
Compare
RUST-1405