Repository: p2panda/aquadoggo (language: rust)

Issue:
Remove duplicate cursor trait  implementation
https://github.com/p2panda/aquadoggo/blob/main/aquadoggo/src/graphql/scalars/cursor_scalar.rs#L39-L51 can be removed


Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cf2a36c68..bba6a7413 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -42,6 +42,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Handle decoding and parsing empty pinned relation lists [#336](https://github.com/p2panda/aquadoggo/pull/336)
 - Make `value` on `QueryRow` an option [#339](https://github.com/p2panda/aquadoggo/pull/339)
 - Fix race conditions in SQLite database test runner [#345](https://github.com/p2panda/aquadoggo/pull/345)
+- Remove duplicate `Cursor` implementation, query code clean up [#346](https://github.com/p2panda/aquadoggo/pull/346)
 - Fix SQL ordering to correctly assemble resulting rows to documents [#347](https://github.com/p2panda/aquadoggo/pull/347)
 
 ## [0.4.0]
diff --git a/aquadoggo/src/db/mod.rs b/aquadoggo/src/db/mod.rs
index 4905b361b..473dea1d5 100644
--- a/aquadoggo/src/db/mod.rs
+++ b/aquadoggo/src/db/mod.rs
@@ -11,7 +11,6 @@ use sqlx::migrate::MigrateDatabase;
 
 pub mod errors;
 pub mod models;
-#[allow(unused)]
 pub mod query;
 pub mod stores;
 pub mod types;
diff --git a/aquadoggo/src/db/query/filter.rs b/aquadoggo/src/db/query/filter.rs
index 68a8dbd2e..0eb00e06f 100644
--- a/aquadoggo/src/db/query/filter.rs
+++ b/aquadoggo/src/db/query/filter.rs
@@ -1,10 +1,9 @@
 // SPDX-License-Identifier: AGPL-3.0-or-later
 
+#[cfg(test)]
 use std::convert::TryInto;
 use std::slice::Iter;
-use std::str::FromStr;
 
-use anyhow::{anyhow, bail, Context, Error, Result};
 use p2panda_rs::operation::OperationValue;
 
 #[cfg(test)]
@@ -107,11 +106,13 @@ impl Filter {
     }
 
     /// Returns the total number of filter settings.
+    #[allow(dead_code)]
     pub fn len(&self) -> usize {
         self.0.len()
     }
 
     /// Returns a filter setting from a given index.
+    #[allow(dead_code)]
     pub fn get(&self, index: usize) -> Option<&FilterSetting> {
         self.0.get(index)
     }
@@ -144,7 +145,7 @@ impl Filter {
         let current_item = self.0.get_mut(index.unwrap()).unwrap();
 
         // Boolean values for the same field we can always easily overwrite
-        if let FilterBy::Element(OperationValue::Boolean(value)) = current_item.by {
+        if let FilterBy::Element(OperationValue::Boolean(_)) = current_item.by {
             *current_item = new_item;
             return;
         }
@@ -366,7 +367,6 @@ impl Filter {
 #[cfg(test)]
 mod tests {
     use p2panda_rs::operation::OperationValue;
-    use p2panda_rs::schema::FieldName;
 
     use crate::db::query::{Field, MetaField};
 
diff --git a/aquadoggo/src/db/query/order.rs b/aquadoggo/src/db/query/order.rs
index 0e0e5caf2..61938e7d6 100644
--- a/aquadoggo/src/db/query/order.rs
+++ b/aquadoggo/src/db/query/order.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: AGPL-3.0-or-later
 
-use crate::db::query::{Field, MetaField};
+use crate::db::query::Field;
 
 /// Options to determine the direction of the ordering.
 #[derive(Debug, Clone, PartialEq)]
@@ -23,6 +23,7 @@ pub struct Order {
 
 impl Order {
     /// Returns a new instance of ordering settings.
+    #[allow(dead_code)]
     pub fn new(field: &Field, direction: &Direction) -> Self {
         Self {
             field: Some(field.clone()),
@@ -42,8 +43,6 @@ impl Default for Order {
 
 #[cfg(test)]
 mod tests {
-    use crate::db::query::{Field, MetaField};
-
     use super::{Direction, Order};
 
     #[test]
diff --git a/aquadoggo/src/db/query/pagination.rs b/aquadoggo/src/db/query/pagination.rs
index fd92e99cb..18dccd238 100644
--- a/aquadoggo/src/db/query/pagination.rs
+++ b/aquadoggo/src/db/query/pagination.rs
@@ -80,6 +80,7 @@ where
     C: Cursor,
 {
     /// Returns a new instance of pagination settings.
+    #[allow(dead_code)]
     pub fn new(first: &NonZeroU64, after: Option<&C>, fields: &Vec<PaginationField>) -> Self {
         Self {
             first: *first,
diff --git a/aquadoggo/src/db/query/select.rs b/aquadoggo/src/db/query/select.rs
index fc00e8ca6..7eae45b0c 100644
--- a/aquadoggo/src/db/query/select.rs
+++ b/aquadoggo/src/db/query/select.rs
@@ -37,11 +37,13 @@ impl Select {
     }
 
     /// Returns the number of selected fields.
+    #[allow(dead_code)]
     pub fn len(&self) -> usize {
         self.fields.len()
     }
 
     /// Returns an iterator over the selected fields.
+    #[allow(dead_code)]
     pub fn iter(&self) -> Iter<Field> {
         self.fields.iter()
     }
diff --git a/aquadoggo/src/db/query/validate.rs b/aquadoggo/src/db/query/validate.rs
index 4053f602b..8b2b2750e 100644
--- a/aquadoggo/src/db/query/validate.rs
+++ b/aquadoggo/src/db/query/validate.rs
@@ -6,10 +6,11 @@ use p2panda_rs::operation::OperationValue;
 use p2panda_rs::schema::{FieldName, FieldType, Schema};
 
 use crate::db::query::errors::QueryError;
-use crate::db::query::{Field, Filter, FilterBy, FilterSetting, MetaField, Order, Select};
+use crate::db::query::{Field, Filter, FilterBy, MetaField, Order, Select};
 
 /// Helper method to make sure that the chosen type in the query value matches the schema's field
 /// type.
+#[allow(dead_code)]
 fn validate_type(
     field_name: &str,
     query_field: &OperationValue,
@@ -30,6 +31,7 @@ fn validate_type(
 ///
 /// Not all fields in a schema can be equally filtered depending on their type. This method makes
 /// sure that these constraints are met and returns an error message.
+#[allow(dead_code)]
 pub fn validate_query(
     select: &Select,
     filter: &Filter,
@@ -172,8 +174,6 @@ pub fn validate_query(
 
 #[cfg(test)]
 mod tests {
-    use p2panda_rs::operation::OperationValue;
-    use p2panda_rs::schema::Schema;
     use rstest::rstest;
 
     use crate::db::query::{Direction, Filter, Order, Select};
diff --git a/aquadoggo/src/db/stores/mod.rs b/aquadoggo/src/db/stores/mod.rs
index 299b0cc94..440b4b3e4 100644
--- a/aquadoggo/src/db/stores/mod.rs
+++ b/aquadoggo/src/db/stores/mod.rs
@@ -6,7 +6,6 @@ pub mod document;
 mod entry;
 mod log;
 mod operation;
-#[allow(unused)]
 mod query;
 mod schema;
 mod task;
diff --git a/aquadoggo/src/db/stores/query.rs b/aquadoggo/src/db/stores/query.rs
index 983817287..bc83875a7 100644
--- a/aquadoggo/src/db/stores/query.rs
+++ b/aquadoggo/src/db/stores/query.rs
@@ -2,17 +2,15 @@
 
 //! This module offers a query API to find many p2panda documents, filtered or sorted by custom
 //! parameters. The results are batched via cursor-based pagination.
-use std::collections::{HashMap, HashSet};
+use std::fmt::Display;
 use std::str::FromStr;
 
 use anyhow::bail;
-use p2panda_rs::document::{DocumentId, DocumentViewFields, DocumentViewId};
-use p2panda_rs::identity::PublicKey;
+use p2panda_rs::document::DocumentViewId;
 use p2panda_rs::operation::OperationValue;
 use p2panda_rs::schema::{FieldName, Schema, SchemaId};
 use p2panda_rs::storage_provider::error::DocumentStorageError;
-use sqlx::any::AnyRow;
-use sqlx::{query, query_as, FromRow};
+use sqlx::query_as;
 
 use crate::db::models::utils::parse_document_view_field_rows;
 use crate::db::models::{DocumentViewFieldRow, QueryRow};
@@ -93,6 +91,12 @@ impl PaginationCursor {
     }
 }
 
+impl Display for PaginationCursor {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.encode())
+    }
+}
+
 // This should _not_ be an underscore character since we're also parsing document view ids in the
 // string which contain that character already
 const CURSOR_SEPARATOR: char = '-';
@@ -586,15 +590,6 @@ fn where_pagination_sql(
     }
 }
 
-fn group_sql(fields: &ApplicationFields) -> String {
-    if fields.is_empty() {
-        // Only one row per document when no field was selected
-        "GROUP BY documents.document_id".to_string()
-    } else {
-        "".to_string()
-    }
-}
-
 fn order_sql(order: &Order, schema: &Schema, list: Option<&RelationList>) -> String {
     // Create custom ordering if query set one
     let custom = order
@@ -858,8 +853,6 @@ impl SqlStore {
         args: &Query<PaginationCursor>,
         list: Option<&RelationList>,
     ) -> Result<QueryResponse, DocumentStorageError> {
-        let schema_id = schema.id();
-
         // Get all selected application fields from query
         let application_fields = args.select.application_fields();
 
@@ -899,7 +892,6 @@ impl SqlStore {
             &args.order,
         );
 
-        let group = group_sql(&application_fields);
         let order = order_sql(&args.order, schema, list);
         let (page_size, limit) = limit_sql(&args.pagination, &application_fields);
 
@@ -1167,16 +1159,13 @@ mod tests {
     use std::num::NonZeroU64;
 
     use p2panda_rs::document::traits::AsDocument;
-    use p2panda_rs::document::{DocumentViewFields, DocumentViewId};
+    use p2panda_rs::document::DocumentViewId;
     use p2panda_rs::hash::Hash;
     use p2panda_rs::identity::KeyPair;
-    use p2panda_rs::operation::{
-        OperationFields, OperationId, OperationValue, PinnedRelationList, Relation,
-    };
+    use p2panda_rs::operation::{OperationValue, PinnedRelationList};
     use p2panda_rs::schema::{FieldType, Schema, SchemaId};
-    use p2panda_rs::test_utils::fixtures::{key_pair, random_hash, schema_id};
+    use p2panda_rs::test_utils::fixtures::{key_pair, schema_id};
     use rstest::rstest;
-    use sqlx::query_as;
 
     use crate::db::models::{OptionalOwner, QueryRow};
     use crate::db::query::{
@@ -1184,7 +1173,6 @@ mod tests {
     };
     use crate::db::stores::{OperationCursor, RelationList};
     use crate::db::types::StorageDocument;
-    use crate::graphql::input_values::OrderDirection;
     use crate::test_utils::{
         add_document, add_schema, add_schema_and_documents, test_runner, TestNode,
     };
@@ -1381,9 +1369,9 @@ mod tests {
         #[case] expected_fields: Vec<OperationValue>,
     ) {
         test_runner(|mut node: TestNode| async move {
-            let (schema, view_ids) = create_events_test_data(&mut node, &key_pair).await;
+            let (schema, _) = create_events_test_data(&mut node, &key_pair).await;
 
-            let (_pagination_data, documents) = node
+            let (_, documents) = node
                 .context
                 .store
                 .query(&schema, &args, None)
@@ -1482,10 +1470,10 @@ mod tests {
     #[rstest]
     fn pinned_relation_list(key_pair: KeyPair) {
         test_runner(|mut node: TestNode| async move {
-            let (venues_schema, mut venues_view_ids) =
+            let (venues_schema, venues_view_ids) =
                 create_venues_test_data(&mut node, &key_pair).await;
 
-            let (visited_schema, mut visited_view_ids) = create_visited_test_data(
+            let (_, visited_view_ids) = create_visited_test_data(
                 &mut node,
                 venues_view_ids,
                 venues_schema.to_owned(),
@@ -1559,8 +1547,7 @@ mod tests {
     #[rstest]
     fn empty_pinned_relation_list(key_pair: KeyPair) {
         test_runner(|mut node: TestNode| async move {
-            let (venues_schema, mut venues_view_ids) =
-                create_venues_test_data(&mut node, &key_pair).await;
+            let (venues_schema, _) = create_venues_test_data(&mut node, &key_pair).await;
 
             let visited_schema = add_schema(
                 &mut node,
@@ -1603,7 +1590,7 @@ mod tests {
             // Select the pinned relation list "venues" for the visited document
             let list = RelationList::new_pinned(&visited_view_id, "venues".into());
 
-            let (pagination_data, documents) = node
+            let (_, documents) = node
                 .context
                 .store
                 .query(&venues_schema, &args, Some(&list))
@@ -1631,7 +1618,7 @@ mod tests {
             // Select the pinned relation list "venues" for the visited document
             let list = RelationList::new_pinned(&visited_view_id, "venues".into());
 
-            let (pagination_data, documents) = node
+            let (_, documents) = node
                 .context
                 .store
                 .query(&venues_schema, &args, Some(&list))
@@ -1640,7 +1627,7 @@ mod tests {
 
             assert!(documents.is_empty());
 
-            // Query selecting application field.
+            // Query selecting application field
             let args = Query::new(
                 &Pagination::new(
                     &NonZeroU64::new(10).unwrap(),
@@ -1656,8 +1643,7 @@ mod tests {
                 &Order::default(),
             );
 
-            let (pagination_data, documents) = node
-                .context
+            node.context
                 .store
                 .query(&visited_schema, &args, None)
                 .await
@@ -1671,7 +1657,7 @@ mod tests {
             let (venues_schema, venues_view_ids) =
                 create_venues_test_data(&mut node, &key_pair).await;
 
-            let (visited_schema, visited_view_ids) = create_visited_test_data(
+            let (_, visited_view_ids) = create_visited_test_data(
                 &mut node,
                 venues_view_ids.clone(),
                 venues_schema.to_owned(),
@@ -1819,10 +1805,10 @@ mod tests {
         #[case] expected_venues: Vec<String>,
     ) {
         test_runner(|mut node: TestNode| async move {
-            let (venues_schema, mut venues_view_ids) =
+            let (venues_schema, venues_view_ids) =
                 create_venues_test_data(&mut node, &key_pair).await;
 
-            let (visited_schema, mut visited_view_ids) = create_visited_test_data(
+            let (_, visited_view_ids) = create_visited_test_data(
                 &mut node,
                 venues_view_ids.clone(),
                 venues_schema.clone(),
@@ -1918,8 +1904,7 @@ mod tests {
     #[case::no_results(Filter::new().fields(&[("name", &["doesnotexist".into()])]), 0)]
     fn count(#[case] filter: Filter, #[case] expected_result: u64, key_pair: KeyPair) {
         test_runner(move |mut node: TestNode| async move {
-            let (venues_schema, mut venues_view_ids) =
-                create_venues_test_data(&mut node, &key_pair).await;
+            let (venues_schema, _) = create_venues_test_data(&mut node, &key_pair).await;
 
             let args = Query::new(
                 &Pagination::default(),
@@ -1950,10 +1935,10 @@ mod tests {
         key_pair: KeyPair,
     ) {
         test_runner(move |mut node: TestNode| async move {
-            let (venues_schema, mut venues_view_ids) =
+            let (venues_schema, venues_view_ids) =
                 create_venues_test_data(&mut node, &key_pair).await;
 
-            let (visited_schema, mut visited_view_ids) = create_visited_test_data(
+            let (_, visited_view_ids) = create_visited_test_data(
                 &mut node,
                 venues_view_ids.clone(),
                 venues_schema.clone(),
@@ -1985,10 +1970,10 @@ mod tests {
     #[rstest]
     fn total_count_of_document_with_relation_list_field(key_pair: KeyPair) {
         test_runner(|mut node: TestNode| async move {
-            let (venues_schema, mut venues_view_ids) =
+            let (venues_schema, venues_view_ids) =
                 create_venues_test_data(&mut node, &key_pair).await;
 
-            let (visited_schema, mut visited_view_ids) = create_visited_test_data(
+            let (visited_schema, _) = create_visited_test_data(
                 &mut node,
                 venues_view_ids,
                 venues_schema.to_owned(),
diff --git a/aquadoggo/src/graphql/scalars/cursor_scalar.rs b/aquadoggo/src/graphql/scalars/cursor_scalar.rs
index 5be5a405b..26e45bd2b 100644
--- a/aquadoggo/src/graphql/scalars/cursor_scalar.rs
+++ b/aquadoggo/src/graphql/scalars/cursor_scalar.rs
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: AGPL-3.0-or-later
 
 use std::fmt::Display;
+use std::str::FromStr;
 
-use anyhow::{anyhow, Error as AnyhowError};
 use async_graphql::{Error, Result};
 use dynamic_graphql::{Scalar, ScalarValue, Value};
 
@@ -20,7 +20,7 @@ impl ScalarValue for CursorScalar {
         Self: Sized,
     {
         match &value {
-            Value::String(str_value) => Ok(Self(PaginationCursor::decode(str_value)?)),
+            Value::String(str_value) => str_value.parse(),
             _ => Err(Error::new(format!("Expected a valid cursor, got: {value}"))),
         }
     }
@@ -30,35 +30,24 @@ impl ScalarValue for CursorScalar {
     }
 }
 
-impl Display for CursorScalar {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}", self.0.encode())
-    }
-}
-
-impl Cursor for CursorScalar {
-    type Error = AnyhowError;
+impl FromStr for CursorScalar {
+    type Err = Error;
 
-    fn decode(value: &str) -> Result<Self, Self::Error> {
-        let value = Value::String(value.to_string());
-
-        Self::from_value(value).map_err(|err| anyhow!(err.message.as_str().to_owned()))
-    }
-
-    fn encode(&self) -> String {
-        self.0.encode()
+    fn from_str(str_value: &str) -> std::result::Result<Self, Self::Err> {
+        let cursor = PaginationCursor::decode(str_value)?;
+        Ok(CursorScalar(cursor))
     }
 }
 
-impl From<&PaginationCursor> for CursorScalar {
-    fn from(cursor: &PaginationCursor) -> Self {
-        Self(cursor.clone())
+impl Display for CursorScalar {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.0)
     }
 }
 
-impl From<&CursorScalar> for PaginationCursor {
-    fn from(cursor: &CursorScalar) -> PaginationCursor {
-        cursor.0.clone()
+impl From<CursorScalar> for PaginationCursor {
+    fn from(cursor: CursorScalar) -> Self {
+        cursor.0
     }
 }
 
diff --git a/aquadoggo/src/graphql/utils.rs b/aquadoggo/src/graphql/utils.rs
index ce1b441fd..5cba5ed68 100644
--- a/aquadoggo/src/graphql/utils.rs
+++ b/aquadoggo/src/graphql/utils.rs
@@ -12,7 +12,7 @@ use p2panda_rs::storage_provider::error::DocumentStorageError;
 use p2panda_rs::storage_provider::traits::DocumentStore;
 
 use crate::db::query::{
-    Cursor, Direction, Field, Filter, MetaField, Order, Pagination, PaginationField, Select,
+    Direction, Field, Filter, MetaField, Order, Pagination, PaginationField, Select,
 };
 use crate::db::stores::{PaginationCursor, Query};
 use crate::db::types::StorageDocument;
@@ -128,8 +128,8 @@ pub fn parse_collection_arguments(
     for (name, value) in ctx.args.iter() {
         match name.as_str() {
             constants::PAGINATION_AFTER_ARG => {
-                let cursor = CursorScalar::decode(value.string()?)?;
-                pagination.after = Some(PaginationCursor::from(&cursor));
+                let cursor: CursorScalar = value.string()?.parse()?;
+                pagination.after = Some(cursor.into());
             }
             constants::PAGINATION_FIRST_ARG => {
                 pagination.first = NonZeroU64::try_from(value.u64()?)?;


Test patch: