RUST-1875 Define separate types for summary and verbose bulk write results #1103
Conversation
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| #[cfg_attr(test, derive(serde::Serialize))] |
There was a problem hiding this comment.
See RUST-1944, we haven't triaged that ticket yet but I think it'd be good to keep test-only serde impls out of the public API
| }; | ||
|
|
||
| #[derive(Clone, Debug, Default)] | ||
| #[non_exhaustive] | ||
| pub struct BulkWriteError { | ||
| pub write_concern_errors: Vec<WriteConcernError>, | ||
| pub write_errors: HashMap<usize, WriteError>, | ||
| pub partial_result: Option<BulkWriteResult>, | ||
| pub partial_result: Option<PartialBulkWriteResult>, |
There was a problem hiding this comment.
This was the sticky part of this work and why I kept it out of the main PR. There were a few ideas I thought of here, the others being:
- Make this
BulkWriteError<R: BulkWriteResult>, storeOption<R>inpartial_result, and defineErrorKind::SummaryBulkWrite(BulkWriteError<SummaryBulkWriteResult>)andErrorKind::VerboseBulkWrite(BulkWriteError<VerboseBulkWriteResult>). This has the benefit of removing indirection in thepartial_resultfield, but I didn't like the duplication inErrorKindboth for internal purposes and API implications. - Define a separate non-sealed
BulkWriteResulttrait with accessor methods and make thepartial_resultfield anOption<Box<dyn BulkWriteResult>>. I was having a hard time getting something like this to compile (lots of object-safety-related errors), and I thinkBox<dyn Trait>is generally kind of annoying to work with so it didn't seem worth it to go down this path.
Matching on PartialBulkWriteResult will be kind of annoying, and it's a bummer that we can't provide the same type guarantees that we can for the actual result types when configuring verbose_results. But I think this will be fine-enough to work with, and we can add accessor methods to the enum directly if this becomes a pain point for users. (FWIW, from the discussions that we had when writing the spec it seems like users are mostly just logging these kinds of error fields.)
src/error/bulk_write.rs
Outdated
| (Self::Summary(this), Self::Summary(other)) => this.merge(other), | ||
| (Self::Verbose(this), Self::Verbose(other)) => this.merge(other), | ||
| // The operation execution path makes this an unreachable state | ||
| _ => {} |
There was a problem hiding this comment.
Maybe unreachable!() here?
src/results/bulk_write.rs
Outdated
| #[cfg_attr(test, serde(rename_all = "camelCase"))] | ||
| #[non_exhaustive] | ||
| pub struct VerboseBulkWriteResult { | ||
| pub inserted_count: i64, |
There was a problem hiding this comment.
I wonder if it'd be worth having pub summary: SummaryBulkWriteResult instead of duplicating the fields? API-wise it makes the superset relationship clear, and it would mean the impl would have a little less repeated code.
There was a problem hiding this comment.
yeah I think that makes sense. this might be the first time I've wanted subclasses in Rust...
No description provided.