From ee99814d4dfc61f511a16d5918a26a9caaddb843 Mon Sep 17 00:00:00 2001 From: Leszek Date: Thu, 28 May 2026 05:04:21 +0200 Subject: [PATCH 1/2] refactor: address code review cleanup --- src/blocking/session.rs | 30 +++---- src/conclusion.rs | 51 ++++++----- src/peer.rs | 188 ++++++++++++++++------------------------ src/session.rs | 78 +++++++++-------- src/types/conclusion.rs | 22 ++++- src/types/dialectic.rs | 11 +++ src/types/pagination.rs | 4 +- src/types/session.rs | 84 +++++++++++------- 8 files changed, 248 insertions(+), 220 deletions(-) diff --git a/src/blocking/session.rs b/src/blocking/session.rs index 81ca1e6..3d78a4f 100644 --- a/src/blocking/session.rs +++ b/src/blocking/session.rs @@ -360,40 +360,38 @@ impl std::fmt::Debug for BlockingUploadFileBuilder<'_> { } impl BlockingUploadFileBuilder<'_> { - /// Set the peer that owns the uploaded file (required). - #[must_use] - pub fn peer(self, id: impl Into) -> Self { + fn with_inner( + self, + f: impl FnOnce(crate::UploadFileBuilder<'_>) -> crate::UploadFileBuilder<'_>, + ) -> Self { Self { - inner: self.inner.peer(id), + inner: f(self.inner), reader_handle: self.reader_handle, } } + /// Set the peer that owns the uploaded file (required). + #[must_use] + pub fn peer(self, id: impl Into) -> Self { + self.with_inner(|b| b.peer(id)) + } + /// Attach arbitrary JSON metadata to the created message(s). #[must_use] pub fn metadata(self, value: Value) -> Self { - Self { - inner: self.inner.metadata(value), - reader_handle: self.reader_handle, - } + self.with_inner(|b| b.metadata(value)) } /// Attach configuration to the created message(s). #[must_use] pub fn configuration(self, value: Value) -> Self { - Self { - inner: self.inner.configuration(value), - reader_handle: self.reader_handle, - } + self.with_inner(|b| b.configuration(value)) } /// Override the creation timestamp (ISO 3339). #[must_use] pub fn created_at(self, dt: DateTime) -> Self { - Self { - inner: self.inner.created_at(dt), - reader_handle: self.reader_handle, - } + self.with_inner(|b| b.created_at(dt)) } /// Send the upload request and return the created messages. diff --git a/src/conclusion.rs b/src/conclusion.rs index 5aad357..f673e0e 100644 --- a/src/conclusion.rs +++ b/src/conclusion.rs @@ -12,6 +12,7 @@ use crate::http::routes; use crate::types::conclusion::Conclusion as ConclusionData; use crate::types::conclusion::ConclusionPage; use crate::types::conclusion::{ConclusionBatchCreate, ConclusionCreate}; +use crate::types::conclusion::{ConclusionFilters, ConclusionGet, ConclusionQuery}; use crate::types::dialectic::RepresentationResponse; use crate::types::pagination::paginate_post; @@ -690,14 +691,19 @@ impl ListConclusionsBuilder { /// # } /// ``` pub async fn send(self) -> Result { - let mut filters = serde_json::json!({ - "observer_id": self.scope.inner.observer, - "observed_id": self.scope.inner.observed, - }); - if let Some(ref sid) = self.session_id { - filters["session_id"] = serde_json::Value::String(sid.clone()); - } - let body = serde_json::json!({"filters": filters}); + let body = ConclusionGet::builder() + .filters( + ConclusionFilters::builder() + .observer_id(self.scope.inner.observer.clone()) + .observed_id(self.scope.inner.observed.clone()) + .maybe_session_id(self.session_id) + .build(), + ) + .build(); + let body = serde_json::to_value(&body).map_err(|e| HonchoError::Decode { + path: "ConclusionGet".to_owned(), + source: e, + })?; let route = routes::conclusions_list(&self.scope.inner.workspace_id)?; paginate_post( &self.scope.inner.http, @@ -781,18 +787,21 @@ impl QueryConclusionsBuilder { "distance must be between 0.0 and 1.0, got {d}" ))); } - let filters = serde_json::json!({ - "observer_id": self.scope.inner.observer, - "observed_id": self.scope.inner.observed, - }); - let mut body = serde_json::json!({ - "query": self.query, - "top_k": self.top_k, - "filters": filters, - }); - if let Some(d) = self.distance { - body["distance"] = serde_json::Value::from(d); - } + let body = ConclusionQuery::builder() + .query(self.query) + .top_k(self.top_k) + .maybe_distance(self.distance) + .filters( + ConclusionFilters::builder() + .observer_id(self.scope.inner.observer.clone()) + .observed_id(self.scope.inner.observed.clone()) + .build(), + ) + .build(); + let body = serde_json::to_value(&body).map_err(|e| HonchoError::Decode { + path: "ConclusionQuery".to_owned(), + source: e, + })?; let route = routes::conclusions_query(&self.scope.inner.workspace_id)?; let data: Vec = self.scope.inner.http.post(&route, Some(&body), &[]).await?; @@ -1249,7 +1258,6 @@ mod tests { let expected_body = serde_json::json!({ "query": "preferences", - "top_k": 10, "filters": { "observer_id": "alice", "observed_id": "bob", @@ -1403,7 +1411,6 @@ mod tests { // Step 3: Query let query_body = serde_json::json!({ "query": "preferences", - "top_k": 10, "filters": { "observer_id": "alice", "observed_id": "bob", diff --git a/src/peer.rs b/src/peer.rs index 37aea99..a9580f5 100644 --- a/src/peer.rs +++ b/src/peer.rs @@ -150,25 +150,7 @@ impl Peer { /// # } /// ``` pub async fn refresh(&self) -> Result<()> { - let mut body_map = serde_json::Map::new(); - body_map.insert("id".into(), Value::String(self.inner.id.clone())); - let body = Value::Object(body_map); - let resp: PeerResponse = self - .inner - .http - .post(&routes::peers(&self.inner.workspace_id)?, Some(&body), &[]) - .await?; - *self - .inner - .metadata - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner) = Some(resp.metadata); - *self - .inner - .configuration - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner) = - map_to_peer_config(&resp.configuration)?; + self.fetch_and_update_cache().await?; Ok(()) } @@ -295,6 +277,11 @@ impl Peer { /// [`PeerConfig`]. #[cfg_attr(feature = "tracing", tracing::instrument(skip(self), fields(peer_id = self.inner.id.as_str())))] pub async fn get_configuration_raw(&self) -> Result> { + let resp = self.fetch_and_update_cache().await?; + Ok(resp.configuration) + } + + async fn fetch_and_update_cache(&self) -> Result { let mut body_map = serde_json::Map::new(); body_map.insert("id".into(), Value::String(self.inner.id.clone())); let body = Value::Object(body_map); @@ -314,7 +301,7 @@ impl Peer { .write() .unwrap_or_else(std::sync::PoisonError::into_inner) = map_to_peer_config(&resp.configuration)?; - Ok(resp.configuration) + Ok(resp) } /// Set the peer's configuration on the server from a raw JSON map. @@ -607,31 +594,26 @@ impl Peer { &self, options: &crate::types::peer::PeerContextOptions, ) -> Result { - let route = routes::peer_context(&self.inner.workspace_id, &self.inner.id)?; - let mut params: Vec<(&str, String)> = Vec::new(); + let mut builder = self.context_builder(); if let Some(ref v) = options.target { - params.push(("target", v.clone())); + builder = builder.target(v.clone()); } - if let Some(ref v) = options.search_query { - params.push(("search_query", v.clone())); + if let Some(v) = options.search_query.clone() { + builder = builder.search_query(v); } - if let Some(ref v) = options.search_top_k { - params.push(("search_top_k", v.to_string())); + if let Some(v) = options.search_top_k { + builder = builder.search_top_k(v); } - if let Some(ref v) = options.search_max_distance { - params.push(("search_max_distance", v.to_string())); + if let Some(v) = options.search_max_distance { + builder = builder.search_max_distance(v); } - if let Some(ref v) = options.include_most_frequent { - params.push(( - "include_most_frequent", - if *v { "true" } else { "false" }.to_string(), - )); + if let Some(v) = options.include_most_frequent { + builder = builder.include_most_frequent(v); } - if let Some(ref v) = options.max_conclusions { - params.push(("max_conclusions", v.to_string())); + if let Some(v) = options.max_conclusions { + builder = builder.max_conclusions(v); } - let refs: Vec<(&str, &str)> = params.iter().map(|(k, v)| (*k, v.as_str())).collect(); - self.inner.http.get(&route, &refs).await + builder.send().await } // ── Sessions ─────────────────────────────────────────────────────── @@ -1195,27 +1177,11 @@ impl RepresentationBuilder { /// or `max_conclusions` are out of range. #[cfg_attr(feature = "tracing", tracing::instrument(skip(self), fields(peer_id = self.peer_id.as_str())))] pub async fn send(self) -> Result { - if let Some(k) = self.search_top_k - && !(1..=100).contains(&k) - { - return Err(HonchoError::Validation(format!( - "search_top_k must be between 1 and 100, got {k}" - ))); - } - if let Some(d) = self.search_max_distance - && !(0.0..=1.0).contains(&d) - { - return Err(HonchoError::Validation(format!( - "search_max_distance must be between 0.0 and 1.0, got {d}" - ))); - } - if let Some(c) = self.max_conclusions - && !(1..=100).contains(&c) - { - return Err(HonchoError::Validation(format!( - "max_conclusions must be between 1 and 100, got {c}" - ))); - } + validate_search_params( + self.search_top_k, + self.search_max_distance, + self.max_conclusions, + )?; let params = crate::types::peer::PeerRepresentationGet { session_id: self.session_id, @@ -1332,60 +1298,30 @@ impl ContextBuilder { /// or `max_conclusions` are out of range. #[cfg_attr(feature = "tracing", tracing::instrument(skip(self), fields(peer_id = self.peer_id.as_str())))] pub async fn send(self) -> Result { - if let Some(k) = self.search_top_k - && !(1..=100).contains(&k) - { - return Err(HonchoError::Validation(format!( - "search_top_k must be between 1 and 100, got {k}" - ))); - } - if let Some(d) = self.search_max_distance - && !(0.0..=1.0).contains(&d) - { - return Err(HonchoError::Validation(format!( - "search_max_distance must be between 0.0 and 1.0, got {d}" - ))); - } - if let Some(c) = self.max_conclusions - && !(1..=100).contains(&c) - { - return Err(HonchoError::Validation(format!( - "max_conclusions must be between 1 and 100, got {c}" - ))); + validate_search_params( + self.search_top_k, + self.search_max_distance, + self.max_conclusions, + )?; + + macro_rules! push_param { + ($params:expr, $key:expr, $val:expr) => { + if let Some(v) = $val { + $params.push(($key, v.to_string())); + } + }; } let route = routes::peer_context(&self.workspace_id, &self.peer_id)?; let mut params: Vec<(&str, String)> = Vec::new(); - if let Some(ref v) = self.target { - params.push(("target", v.clone())); - } - if let Some(v) = self.summary { - params.push(("summary", if v { "true" } else { "false" }.to_string())); - } - if let Some(v) = self.limit_to_session { - params.push(( - "limit_to_session", - if v { "true" } else { "false" }.to_string(), - )); - } - if let Some(ref v) = self.search_query { - params.push(("search_query", v.clone())); - } - if let Some(v) = self.search_top_k { - params.push(("search_top_k", v.to_string())); - } - if let Some(v) = self.search_max_distance { - params.push(("search_max_distance", v.to_string())); - } - if let Some(v) = self.include_most_frequent { - params.push(( - "include_most_frequent", - if v { "true" } else { "false" }.to_string(), - )); - } - if let Some(v) = self.max_conclusions { - params.push(("max_conclusions", v.to_string())); - } + push_param!(params, "target", self.target); + push_param!(params, "summary", self.summary); + push_param!(params, "limit_to_session", self.limit_to_session); + push_param!(params, "search_query", self.search_query); + push_param!(params, "search_top_k", self.search_top_k); + push_param!(params, "search_max_distance", self.search_max_distance); + push_param!(params, "include_most_frequent", self.include_most_frequent); + push_param!(params, "max_conclusions", self.max_conclusions); let refs: Vec<(&str, &str)> = params.iter().map(|(k, v)| (*k, v.as_str())).collect(); self.http.get(&route, &refs).await } @@ -1489,9 +1425,39 @@ impl MessageBuilder { } } +fn validate_search_params( + search_top_k: Option, + search_max_distance: Option, + max_conclusions: Option, +) -> Result<()> { + if let Some(k) = search_top_k + && !(1..=100).contains(&k) + { + return Err(HonchoError::Validation(format!( + "search_top_k must be between 1 and 100, got {k}" + ))); + } + if let Some(d) = search_max_distance + && !(0.0..=1.0).contains(&d) + { + return Err(HonchoError::Validation(format!( + "search_max_distance must be between 0.0 and 1.0, got {d}" + ))); + } + if let Some(c) = max_conclusions + && !(1..=100).contains(&c) + { + return Err(HonchoError::Validation(format!( + "max_conclusions must be between 1 and 100, got {c}" + ))); + } + Ok(()) +} + fn map_to_peer_config(map: &HashMap) -> Result> { - let val = serde_json::to_value(map).map_err(|e| HonchoError::Configuration(e.to_string()))?; - serde_json::from_value(val) + let obj: serde_json::Map = + map.iter().map(|(k, v)| (k.clone(), v.clone())).collect(); + serde_json::from_value(Value::Object(obj)) .map(Some) .map_err(|e| HonchoError::Configuration(e.to_string())) } diff --git a/src/session.rs b/src/session.rs index b994a47..061a55a 100644 --- a/src/session.rs +++ b/src/session.rs @@ -23,6 +23,11 @@ use crate::upload::FileSource; pub(crate) struct SessionInner { http: HttpClient, + // PERF: workspace_id is cloned per Message/Peer/Session creation (~11 call sites). + // Switching to Arc would eliminate String clones but ripples through Honcho, + // Peer, Session, Message, Conclusion, and all builder types. The current cost is + // negligible for typical workloads (<100 messages). Revisit if profiling shows + // allocation pressure in hot loops (e.g., bulk message import). workspace_id: String, id: String, is_active: AtomicBool, @@ -116,6 +121,43 @@ pub struct UploadFileBuilder<'a> { created_at: Option>, } +fn serialize_upload_fields( + builder: &UploadFileBuilder<'_>, +) -> Result Form + Clone + Send + 'static> { + let metadata_text = builder + .metadata + .as_ref() + .map(|md| { + serde_json::to_string(md) + .map_err(|e| HonchoError::Configuration(format!("metadata: {e}"))) + }) + .transpose()?; + + let configuration_text = builder + .configuration + .as_ref() + .map(|cfg| { + serde_json::to_string(cfg) + .map_err(|e| HonchoError::Configuration(format!("configuration: {e}"))) + }) + .transpose()?; + + let created_at_text = builder.created_at.map(|dt| dt.to_rfc3339()); + + Ok(move |mut form: Form| -> Form { + if let Some(ref md) = metadata_text { + form = form.text("metadata", md.clone()); + } + if let Some(ref cfg) = configuration_text { + form = form.text("configuration", cfg.clone()); + } + if let Some(ref dt) = created_at_text { + form = form.text("created_at", dt.clone()); + } + form + }) +} + impl UploadFileBuilder<'_> { /// Set the peer that owns the uploaded file (required). /// @@ -202,8 +244,9 @@ impl UploadFileBuilder<'_> { feature = "tracing", tracing::instrument(skip(self), name = "upload_file_send") )] - #[allow(clippy::too_many_lines)] pub async fn send(self) -> Result> { + let add_text_fields = serialize_upload_fields(&self)?; + let peer_id = self .peer_id .ok_or_else(|| HonchoError::Validation("peer_id is required".into()))?; @@ -212,39 +255,6 @@ impl UploadFileBuilder<'_> { .source .ok_or_else(|| HonchoError::Validation("file source is required".into()))?; - let metadata_text = self - .metadata - .as_ref() - .map(|md| { - serde_json::to_string(md) - .map_err(|e| HonchoError::Configuration(format!("metadata: {e}"))) - }) - .transpose()?; - - let configuration_text = self - .configuration - .as_ref() - .map(|cfg| { - serde_json::to_string(cfg) - .map_err(|e| HonchoError::Configuration(format!("configuration: {e}"))) - }) - .transpose()?; - - let created_at_text = self.created_at.map(|dt| dt.to_rfc3339()); - - let add_text_fields = move |mut form: Form| -> Form { - if let Some(ref md) = metadata_text { - form = form.text("metadata", md.clone()); - } - if let Some(ref cfg) = configuration_text { - form = form.text("configuration", cfg.clone()); - } - if let Some(ref dt) = created_at_text { - form = form.text("created_at", dt.clone()); - } - form - }; - // FileSource::Path — Part::file() streams from disk, re-opens on retry. // FileSource::Stream — must buffer: AsyncRead consumed once, not replayable. // FileSource::Bytes — already in memory. diff --git a/src/types/conclusion.rs b/src/types/conclusion.rs index 01b2d15..6ada8ac 100644 --- a/src/types/conclusion.rs +++ b/src/types/conclusion.rs @@ -3,7 +3,6 @@ //! Maps the `OpenAPI` schemas: `Conclusion`, `ConclusionCreate`, //! `ConclusionBatchCreate`, `ConclusionGet`, `ConclusionQuery`, `Page[Conclusion]`. -use crate::types::common::JsonValue; use crate::types::pagination::Page; use chrono::{DateTime, Utc}; @@ -60,6 +59,23 @@ pub struct ConclusionBatchCreate { pub conclusions: Vec, } +/// Typed filters for conclusion list and query requests. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, bon::Builder)] +#[builder(on(String, into))] +#[builder(finish_fn = build)] +#[non_exhaustive] +pub struct ConclusionFilters { + /// Filter by observer peer ID. + #[serde(skip_serializing_if = "Option::is_none")] + pub observer_id: Option, + /// Filter by observed peer ID. + #[serde(skip_serializing_if = "Option::is_none")] + pub observed_id: Option, + /// Optional session ID filter. + #[serde(skip_serializing_if = "Option::is_none")] + pub session_id: Option, +} + /// Request body for listing conclusions with optional filters. /// /// Maps `OpenAPI` `ConclusionGet`. @@ -69,7 +85,7 @@ pub struct ConclusionBatchCreate { pub struct ConclusionGet { /// Optional metadata filters. #[serde(skip_serializing_if = "Option::is_none")] - pub filters: Option, + pub filters: Option, } /// Request body for semantic search over conclusions. @@ -90,7 +106,7 @@ pub struct ConclusionQuery { pub distance: Option, /// Additional metadata filters. #[serde(skip_serializing_if = "Option::is_none")] - pub filters: Option, + pub filters: Option, } fn default_top_k() -> u32 { diff --git a/src/types/dialectic.rs b/src/types/dialectic.rs index c9bfd70..c268815 100644 --- a/src/types/dialectic.rs +++ b/src/types/dialectic.rs @@ -79,6 +79,17 @@ pub fn validate_dialectic_query(query: &str) -> Result<()> { impl DialecticOptions { /// Validate options before sending them to the API. + /// + /// This is a separate method (not part of the builder's `build()`) because + /// `bon::Builder` with `finish_fn = build` does not support fallible finish. + /// Call this after `.build()` and before passing the options to the API: + /// + /// ```ignore + /// let opts = DialecticOptions::builder() + /// .query("hello") + /// .build() + /// .validate()?; + /// ``` pub fn validate(&self) -> Result<()> { validate_dialectic_query(&self.query) } diff --git a/src/types/pagination.rs b/src/types/pagination.rs index 95752ef..ccdd636 100644 --- a/src/types/pagination.rs +++ b/src/types/pagination.rs @@ -429,12 +429,12 @@ where pub(crate) fn validate_pagination(page: u64, size: u64) -> Result<()> { if page == 0 { return Err(HonchoError::Validation( - "page must be greater than or equal to 1".to_string(), + "page must be greater than or equal to 1".into(), )); } if !(1..=100).contains(&size) { return Err(HonchoError::Validation( - "size must be between 1 and 100".to_string(), + "size must be between 1 and 100".into(), )); } Ok(()) diff --git a/src/types/session.rs b/src/types/session.rs index d15a210..87ea69b 100644 --- a/src/types/session.rs +++ b/src/types/session.rs @@ -49,6 +49,27 @@ pub struct SessionCreate { pub configuration: Option, } +impl SessionCreate { + /// Validate session ID constraints. + pub fn validate(&self) -> std::result::Result<(), crate::error::HonchoError> { + if self.id.is_empty() { + return Err(crate::error::HonchoError::Validation( + "session id must not be empty".into(), + )); + } + if !self + .id + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') + { + return Err(crate::error::HonchoError::Validation( + "session id must contain only [a-zA-Z0-9_-]".into(), + )); + } + Ok(()) + } +} + /// Request body for updating a session. #[non_exhaustive] #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, bon::Builder)] @@ -349,6 +370,33 @@ impl IntoAssistantRef for &crate::Peer { } impl SessionContext { + /// Format a peer card into a single displayable string. + fn format_peer_card(card: &[String]) -> String { + let items: Vec = card + .iter() + .map(|s| format!("'{}'", s.replace('\'', "\\'"))) + .collect(); + format!("[{}]", items.join(", ")) + } + + /// Build context system messages shared across provider formats. + /// + /// Returns `(content_tag, content_value)` pairs for `peer_representation`, + /// `peer_card`, and `summary`, in that order. + fn build_context_messages(&self) -> Vec<(&'static str, String)> { + let mut msgs = Vec::new(); + if let Some(ref rep) = self.peer_representation { + msgs.push(("peer_representation", rep.clone())); + } + if let Some(ref card) = self.peer_card { + msgs.push(("peer_card", Self::format_peer_card(card))); + } + if let Some(ref summary) = self.summary { + msgs.push(("summary", summary.content.clone())); + } + msgs + } + /// Convert the context to OpenAI-compatible message format. /// /// System messages (`peer_representation`, `peer_card`, summary) are prepended. @@ -372,24 +420,10 @@ impl SessionContext { let assistant = assistant.as_assistant_name(); let mut result: Vec = Vec::new(); - if let Some(ref rep) = self.peer_representation { + for (tag, value) in self.build_context_messages() { result.push(serde_json::json!({ "role": "system", - "content": format!("{rep}"), - })); - } - - if let Some(ref card) = self.peer_card { - result.push(serde_json::json!({ - "role": "system", - "content": format!("[{}]", card.iter().map(|s| format!("'{}'", s.replace('\'', "\\'"))).collect::>().join(", ")), - })); - } - - if let Some(ref summary) = self.summary { - result.push(serde_json::json!({ - "role": "system", - "content": format!("{}", summary.content), + "content": format!("<{tag}>{value}"), })); } @@ -426,24 +460,10 @@ impl SessionContext { let assistant = assistant.as_assistant_name(); let mut result: Vec = Vec::new(); - if let Some(ref rep) = self.peer_representation { - result.push(serde_json::json!({ - "role": "user", - "content": format!("{rep}"), - })); - } - - if let Some(ref card) = self.peer_card { - result.push(serde_json::json!({ - "role": "user", - "content": format!("[{}]", card.iter().map(|s| format!("'{}'", s.replace('\'', "\\'"))).collect::>().join(", ")), - })); - } - - if let Some(ref summary) = self.summary { + for (tag, value) in self.build_context_messages() { result.push(serde_json::json!({ "role": "user", - "content": format!("{}", summary.content), + "content": format!("<{tag}>{value}"), })); } From 3df379b034e84b6c90bbad6a85c027a6a466aa04 Mon Sep 17 00:00:00 2001 From: Leszek Date: Thu, 28 May 2026 05:49:53 +0200 Subject: [PATCH 2/2] perf: eliminate unnecessary String clones in hot paths - ContextBuilder::send: move owned String params directly instead of calling to_string() via macro - PeerContextOptions: clone only when Some, not the entire Option - build_context_messages: return Cow to borrow existing strings instead of cloning - UploadFileBuilder::send: validate required fields before serialize_upload_fields so error priority matches user expectation --- src/peer.rs | 12 ++++++++---- src/session.rs | 19 ++++++++++++------- src/types/session.rs | 17 +++++++++++++---- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/peer.rs b/src/peer.rs index a9580f5..695f380 100644 --- a/src/peer.rs +++ b/src/peer.rs @@ -598,8 +598,8 @@ impl Peer { if let Some(ref v) = options.target { builder = builder.target(v.clone()); } - if let Some(v) = options.search_query.clone() { - builder = builder.search_query(v); + if let Some(ref v) = options.search_query { + builder = builder.search_query(v.clone()); } if let Some(v) = options.search_top_k { builder = builder.search_top_k(v); @@ -1314,10 +1314,14 @@ impl ContextBuilder { let route = routes::peer_context(&self.workspace_id, &self.peer_id)?; let mut params: Vec<(&str, String)> = Vec::new(); - push_param!(params, "target", self.target); + if let Some(v) = self.target { + params.push(("target", v)); + } push_param!(params, "summary", self.summary); push_param!(params, "limit_to_session", self.limit_to_session); - push_param!(params, "search_query", self.search_query); + if let Some(v) = self.search_query { + params.push(("search_query", v)); + } push_param!(params, "search_top_k", self.search_top_k); push_param!(params, "search_max_distance", self.search_max_distance); push_param!(params, "include_most_frequent", self.include_most_frequent); diff --git a/src/session.rs b/src/session.rs index 061a55a..119c5da 100644 --- a/src/session.rs +++ b/src/session.rs @@ -244,16 +244,21 @@ impl UploadFileBuilder<'_> { feature = "tracing", tracing::instrument(skip(self), name = "upload_file_send") )] + #[allow(clippy::missing_panics_doc)] pub async fn send(self) -> Result> { - let add_text_fields = serialize_upload_fields(&self)?; + if self.peer_id.is_none() { + return Err(HonchoError::Validation("peer_id is required".into())); + } + if self.source.is_none() { + return Err(HonchoError::Validation("file source is required".into())); + } - let peer_id = self - .peer_id - .ok_or_else(|| HonchoError::Validation("peer_id is required".into()))?; + let add_text_fields = serialize_upload_fields(&self)?; - let source = self - .source - .ok_or_else(|| HonchoError::Validation("file source is required".into()))?; + #[allow(clippy::expect_used)] + let peer_id = self.peer_id.expect("peer_id validated above"); + #[allow(clippy::expect_used)] + let source = self.source.expect("source validated above"); // FileSource::Path — Part::file() streams from disk, re-opens on retry. // FileSource::Stream — must buffer: AsyncRead consumed once, not replayable. diff --git a/src/types/session.rs b/src/types/session.rs index 87ea69b..777fb97 100644 --- a/src/types/session.rs +++ b/src/types/session.rs @@ -383,16 +383,25 @@ impl SessionContext { /// /// Returns `(content_tag, content_value)` pairs for `peer_representation`, /// `peer_card`, and `summary`, in that order. - fn build_context_messages(&self) -> Vec<(&'static str, String)> { + fn build_context_messages(&self) -> Vec<(&'static str, std::borrow::Cow<'_, str>)> { let mut msgs = Vec::new(); if let Some(ref rep) = self.peer_representation { - msgs.push(("peer_representation", rep.clone())); + msgs.push(( + "peer_representation", + std::borrow::Cow::Borrowed(rep.as_str()), + )); } if let Some(ref card) = self.peer_card { - msgs.push(("peer_card", Self::format_peer_card(card))); + msgs.push(( + "peer_card", + std::borrow::Cow::Owned(Self::format_peer_card(card)), + )); } if let Some(ref summary) = self.summary { - msgs.push(("summary", summary.content.clone())); + msgs.push(( + "summary", + std::borrow::Cow::Borrowed(summary.content.as_str()), + )); } msgs }