Skip to content

Commit 695e9c4

Browse files
saghmsaghm
committed
RUST-679 save error messages rather than strings for internal SDAM errors
1 parent 513f708 commit 695e9c4

File tree

4 files changed

+44
-36
lines changed

4 files changed

+44
-36
lines changed

‎src/sdam/description/server.rs

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ use chrono::offset::Utc;
55

66
use crate::{
77
client::ClusterTime,
8-
error::Result,
98
is_master::IsMasterReply,
109
options::StreamAddress,
1110
selection_criteria::TagSet,
@@ -74,7 +73,7 @@ pub(crate) struct ServerDescription {
7473
// allows us to ensure that only valid states are possible (e.g. preventing that both an error
7574
// and a reply are present) while still making it easy to define helper methods on
7675
// ServerDescription for information we need from the isMaster reply by propagating with `?`.
77-
pub(crate) reply: Result<Option<IsMasterReply>>,
76+
pub(crate) reply: Result<Option<IsMasterReply>, String>,
7877
}
7978

8079
impl PartialEq for ServerDescription {
@@ -98,7 +97,7 @@ impl PartialEq for ServerDescription {
9897
impl ServerDescription {
9998
pub(crate) fn new(
10099
mut address: StreamAddress,
101-
is_master_reply: Option<Result<IsMasterReply>>,
100+
is_master_reply: Option<Result<IsMasterReply, String>>,
102101
) -> Self {
103102
address.hostname = address.hostname.to_lowercase();
104103

@@ -201,7 +200,7 @@ impl ServerDescription {
201200
None
202201
}
203202

204-
pub(crate) fn set_name(&self) -> Result<Option<String>> {
203+
pub(crate) fn set_name(&self) -> Result<Option<String>, String> {
205204
let set_name = self
206205
.reply
207206
.as_ref()
@@ -211,7 +210,7 @@ impl ServerDescription {
211210
Ok(set_name)
212211
}
213212

214-
pub(crate) fn known_hosts(&self) -> Result<impl Iterator<Item = &String>> {
213+
pub(crate) fn known_hosts(&self) -> Result<impl Iterator<Item = &String>, String> {
215214
let known_hosts = self
216215
.reply
217216
.as_ref()
@@ -232,7 +231,7 @@ impl ServerDescription {
232231
Ok(known_hosts.into_iter().flatten())
233232
}
234233

235-
pub(crate) fn invalid_me(&self) -> Result<bool> {
234+
pub(crate) fn invalid_me(&self) -> Result<bool, String> {
236235
if let Some(ref reply) = self.reply.as_ref().map_err(Clone::clone)? {
237236
if let Some(ref me) = reply.command_response.me {
238237
return Ok(&self.address.to_string() != me);
@@ -242,7 +241,7 @@ impl ServerDescription {
242241
Ok(false)
243242
}
244243

245-
pub(crate) fn set_version(&self) -> Result<Option<i32>> {
244+
pub(crate) fn set_version(&self) -> Result<Option<i32>, String> {
246245
let me = self
247246
.reply
248247
.as_ref()
@@ -252,7 +251,7 @@ impl ServerDescription {
252251
Ok(me)
253252
}
254253

255-
pub(crate) fn election_id(&self) -> Result<Option<ObjectId>> {
254+
pub(crate) fn election_id(&self) -> Result<Option<ObjectId>, String> {
256255
let me = self
257256
.reply
258257
.as_ref()
@@ -263,7 +262,7 @@ impl ServerDescription {
263262
}
264263

265264
#[cfg(test)]
266-
pub(crate) fn min_wire_version(&self) -> Result<Option<i32>> {
265+
pub(crate) fn min_wire_version(&self) -> Result<Option<i32>, String> {
267266
let me = self
268267
.reply
269268
.as_ref()
@@ -274,7 +273,7 @@ impl ServerDescription {
274273
}
275274

276275
#[cfg(test)]
277-
pub(crate) fn max_wire_version(&self) -> Result<Option<i32>> {
276+
pub(crate) fn max_wire_version(&self) -> Result<Option<i32>, String> {
278277
let me = self
279278
.reply
280279
.as_ref()
@@ -284,7 +283,7 @@ impl ServerDescription {
284283
Ok(me)
285284
}
286285

287-
pub(crate) fn last_write_date(&self) -> Result<Option<DateTime>> {
286+
pub(crate) fn last_write_date(&self) -> Result<Option<DateTime>, String> {
288287
match self.reply {
289288
Ok(None) => Ok(None),
290289
Ok(Some(ref reply)) => Ok(reply
@@ -296,7 +295,7 @@ impl ServerDescription {
296295
}
297296
}
298297

299-
pub(crate) fn logical_session_timeout(&self) -> Result<Option<Duration>> {
298+
pub(crate) fn logical_session_timeout(&self) -> Result<Option<Duration>, String> {
300299
match self.reply {
301300
Ok(None) => Ok(None),
302301
Ok(Some(ref reply)) => Ok(reply
@@ -307,7 +306,7 @@ impl ServerDescription {
307306
}
308307
}
309308

310-
pub(crate) fn cluster_time(&self) -> Result<Option<ClusterTime>> {
309+
pub(crate) fn cluster_time(&self) -> Result<Option<ClusterTime>, String> {
311310
match self.reply {
312311
Ok(None) => Ok(None),
313312
Ok(Some(ref reply)) => Ok(reply.cluster_time.clone()),

‎src/sdam/description/topology/mod.rs

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ use crate::{
1313
bson::oid::ObjectId,
1414
client::ClusterTime,
1515
cmap::Command,
16-
error::{ErrorKind, Result},
1716
options::{ClientOptions, StreamAddress},
1817
sdam::description::server::{ServerDescription, ServerType},
1918
selection_criteria::{ReadPreference, SelectionCriteria},
@@ -111,7 +110,7 @@ impl TopologyDescription {
111110
}
112111
}
113112

114-
pub(crate) fn new(options: ClientOptions) -> Result<Self> {
113+
pub(crate) fn new(options: ClientOptions) -> crate::error::Result<Self> {
115114
verify_max_staleness(
116115
options
117116
.selection_criteria
@@ -395,7 +394,10 @@ impl TopologyDescription {
395394

396395
/// Update the topology based on the new information about the topology contained by the
397396
/// ServerDescription.
398-
pub(crate) fn update(&mut self, mut server_description: ServerDescription) -> Result<()> {
397+
pub(crate) fn update(
398+
&mut self,
399+
mut server_description: ServerDescription,
400+
) -> Result<(), String> {
399401
// Ignore updates from servers not currently in the cluster.
400402
if !self.servers.contains_key(&server_description.address) {
401403
return Ok(());
@@ -439,7 +441,10 @@ impl TopologyDescription {
439441
}
440442

441443
/// Update the Unknown topology description based on the server description.
442-
fn update_unknown_topology(&mut self, server_description: ServerDescription) -> Result<()> {
444+
fn update_unknown_topology(
445+
&mut self,
446+
server_description: ServerDescription,
447+
) -> Result<(), String> {
443448
match server_description.server_type {
444449
ServerType::Unknown | ServerType::RSGhost => {}
445450
ServerType::Standalone => {
@@ -473,7 +478,7 @@ impl TopologyDescription {
473478
fn update_replica_set_no_primary_topology(
474479
&mut self,
475480
server_description: ServerDescription,
476-
) -> Result<()> {
481+
) -> Result<(), String> {
477482
match server_description.server_type {
478483
ServerType::Unknown | ServerType::RSGhost => {}
479484
ServerType::Standalone | ServerType::Mongos => {
@@ -495,7 +500,7 @@ impl TopologyDescription {
495500
fn update_replica_set_with_primary_topology(
496501
&mut self,
497502
server_description: ServerDescription,
498-
) -> Result<()> {
503+
) -> Result<(), String> {
499504
match server_description.server_type {
500505
ServerType::Unknown | ServerType::RSGhost => {
501506
self.record_primary_state();
@@ -527,7 +532,7 @@ impl TopologyDescription {
527532
fn update_rs_without_primary_server(
528533
&mut self,
529534
server_description: ServerDescription,
530-
) -> Result<()> {
535+
) -> Result<(), String> {
531536
if self.set_name.is_none() {
532537
self.set_name = server_description.set_name()?;
533538
} else if self.set_name != server_description.set_name()? {
@@ -550,7 +555,7 @@ impl TopologyDescription {
550555
fn update_rs_with_primary_from_member(
551556
&mut self,
552557
server_description: ServerDescription,
553-
) -> Result<()> {
558+
) -> Result<(), String> {
554559
if self.set_name != server_description.set_name()? {
555560
self.servers.remove(&server_description.address);
556561
self.record_primary_state();
@@ -572,7 +577,7 @@ impl TopologyDescription {
572577
fn update_rs_from_primary_server(
573578
&mut self,
574579
server_description: ServerDescription,
575-
) -> Result<()> {
580+
) -> Result<(), String> {
576581
if self.set_name.is_none() {
577582
self.set_name = server_description.set_name()?;
578583
} else if self.set_name != server_description.set_name()? {
@@ -661,8 +666,13 @@ impl TopologyDescription {
661666
}
662667

663668
/// Create a new ServerDescription for each address and add it to the topology.
664-
fn add_new_servers<'a>(&mut self, servers: impl Iterator<Item = &'a String>) -> Result<()> {
665-
let servers: Result<Vec<_>> = servers.map(|server| StreamAddress::parse(server)).collect();
669+
fn add_new_servers<'a>(
670+
&mut self,
671+
servers: impl Iterator<Item = &'a String>,
672+
) -> Result<(), String> {
673+
let servers: Result<Vec<_>, String> = servers
674+
.map(|server| StreamAddress::parse(server).map_err(|e| e.to_string()))
675+
.collect();
666676

667677
self.add_new_servers_from_addresses(servers?.iter());
668678
Ok(())
@@ -731,15 +741,17 @@ pub(crate) struct TopologyDescriptionDiff {
731741
pub(crate) new_addresses: HashSet<StreamAddress>,
732742
}
733743

734-
fn verify_max_staleness(max_staleness: Option<Duration>) -> Result<()> {
744+
fn verify_max_staleness(max_staleness: Option<Duration>) -> crate::error::Result<()> {
745+
verify_max_staleness_inner(max_staleness)
746+
.map_err(|s| crate::error::ErrorKind::ArgumentError { message: s }.into())
747+
}
748+
749+
fn verify_max_staleness_inner(max_staleness: Option<Duration>) -> Result<(), String> {
735750
if max_staleness
736751
.map(|staleness| staleness > Duration::from_secs(0) && staleness < Duration::from_secs(90))
737752
.unwrap_or(false)
738753
{
739-
return Err(ErrorKind::ArgumentError {
740-
message: "max staleness cannot be both positive and below 90 seconds".into(),
741-
}
742-
.into());
754+
return Err("max staleness cannot be both positive and below 90 seconds".into());
743755
}
744756

745757
Ok(())

‎src/sdam/description/topology/test/sdam.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ use tokio::sync::RwLockReadGuard;
66
use crate::{
77
bson::{doc, oid::ObjectId},
88
client::Client,
9-
error::ErrorKind,
109
is_master::{IsMasterCommandResponse, IsMasterReply},
1110
options::{ClientOptions, ReadPreference, SelectionCriteria, StreamAddress},
1211
sdam::description::{
@@ -82,10 +81,7 @@ async fn run_test(test_file: TestFile) {
8281
for (i, phase) in test_file.phases.into_iter().enumerate() {
8382
for Response(address, command_response) in phase.responses {
8483
let is_master_reply = if command_response == Default::default() {
85-
Err(ErrorKind::OperationError {
86-
message: "dummy error".to_string(),
87-
}
88-
.into())
84+
Err("dummy error".to_string())
8985
} else {
9086
Ok(IsMasterReply {
9187
command_response,

‎src/sdam/state/mod.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,8 @@ impl Topology {
283283
server: &Server,
284284
state_lock: RwLockWriteGuard<'_, TopologyState>,
285285
) -> bool {
286-
let description = ServerDescription::new(server.address.clone(), Some(Err(error)));
286+
let description =
287+
ServerDescription::new(server.address.clone(), Some(Err(error.to_string())));
287288
self.update_and_notify(server, description, state_lock)
288289
.await
289290
}
@@ -470,7 +471,7 @@ impl TopologyState {
470471
server: ServerDescription,
471472
options: &ClientOptions,
472473
topology: WeakTopology,
473-
) -> Result<Option<TopologyDescriptionDiff>> {
474+
) -> std::result::Result<Option<TopologyDescriptionDiff>, String> {
474475
let old_description = self.description.clone();
475476
self.description.update(server)?;
476477

0 commit comments

Comments
 (0)