Feat/add findex redis v7 #20

HatemMn · 2024-12-03T13:06:04Z

Upgrade findex to the v7 version but keep the same usages of the server
Purge cloudproof : delete all cloudproof imports and references, replacing them with direct import from findex repo when necessary
Make sure the DB operations are atomic and asyncronous. For reference, a PR is made on the redis crate : Add transaction_async function redis-rs/redis-rs#1485
Minor coding style updates
Create a serialization for findex structs <=> byte streams. Write unit tests for those operations. All this is centralized under crate/structs . This will be optimized later on : Deserializer, Serializer #29

crate/server/src/database/database_traits.rs

crate/server/src/routes/findex.rs

HatemMn · 2024-12-26T16:09:06Z

crate/cli/src/tests/datasets.rs

+}
+
+#[allow(clippy::indexing_slicing)]
+fn parse_entries(s: &str) -> CliResult<EncryptedEntries> {


Please confirm me that this is the right way to parse findex result. It works yes, but idk if my implementation is case specefic or would work on other datasets

@tbrezot what's your opinion abt this function ?

crate/cli/src/actions/datasets.rs

tbrezot

Many todos, unnecessary clones and "tooling" diffs that I would have preferred not to see here: keep functional changes separate to ease reviewing.

This first review will be followed by a more in-depth one when I have time.

Cargo.toml

crate/cli/src/actions/findex/search.rs

crate/server/src/database/redis/findex.rs

crate/server/src/error/server.rs

crate/cli/src/commands.rs

crate/server/src/routes/utils.rs

tbrezot · 2025-01-03T16:25:31Z

crate/structs/Cargo.toml

+cosmian_crypto_core = { workspace = true, features = ["ser"] }
+cosmian_findex = { workspace = true }
+cosmian_findex_config = { path = "../config" }
+rand = "0.8.5"


What is rand used for ?

documentation/build_pdf.sh

crate/server/src/database/redis/permissions.rs

Manuthor

partial review only

.rustfmt.toml

.github/scripts/cargo_build.ps1

Cargo.toml

crate/cli/src/actions/console.rs

crate/cli/src/tests/datasets.rs

crate/client/src/lib.rs

crate/config/Cargo.toml

rust-toolchain

HatemMn · 2025-01-08T10:12:59Z

crate/server/src/core/implementation.rs

 pub(crate) struct FindexServer {
    pub(crate) params: ServerParams,
-    pub(crate) db: Box<dyn DatabaseTraits + Sync + Send>,
+    pub(crate) db: Redis<WORD_LENGTH>,


INFO : I had to take this out because batch_read and guarded write are not compatible with Box<dyn ...>

pub(crate) db: Box<dyn DatabaseTraits + Sync + Send>,

I tried various fixes, didn't work. imo it's BR & GW that should be changed if we want to keep this as box.

Here is the error :

the trait `DatabaseTraits` cannot be made into an object consider moving `batch_read` to another trait consider moving `guarded_write` to another trait only type `database::redis::instance::Redis<WORD_LENGTH>` implements the trait, consider using it directly insteadre

HatemMn · 2025-01-08T10:44:12Z

crate/server/src/database/redis/permissions.rs

    async fn create_index_id(&self, user_id: &str) -> FResult<Uuid> {
        let redis_key = user_id.as_bytes();

-        let mut con = self.client.get_connection()?;
+        // let mut con = self.client.get_connection()?;
+        let mut con: redis::aio::ConnectionManager = self.memory.manager.clone();


no more need to get the connection "out" of the ConnectionManager. The ConnectionManager implements ConnectionLike trait and can be used directly to execute Redis commands. It handles the connection management internally.

HatemMn · 2025-01-08T14:46:59Z

crate/server/src/database/redis/permissions.rs

+        pipe.atomic()
+            .query_async::<()>(&mut self.memory.manager.clone())
+            .await
+            .map_err(FindexServerError::from)?;



writing let _:() is a fix for this error :

error: this function depends on never type fallback being `()` --> crate/server/src/database/redis/permissions.rs:66:5 | 66 | #[instrument(ret(Display), err, skip(self), level = "trace")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #123748 <https://github.com/rust-lang/rust/issues/123748> = help: specify the types explicitly note: in edition 2024, the requirement `!: FromRedisValue` will fail --> crate/server/src/database/redis/permissions.rs:79:18 | 79 | .query_async(&mut con_manager) | ^^^^^^^^^^^ note: the lint level is defined here --> crate/server/src/lib.rs:7:5 | 7 | future_incompatible, | ^^^^^^^^^^^^^^^^^^^ = note: `#[deny(dependency_on_unit_never_type_fallback)]` implied by `#[deny(future_incompatible)]` = note: this error originates in the attribute macro `instrument` (in Nightly builds, run with -Z macro-backtrace for more info)

Cargo.toml

crate/cli/src/actions/findex/structs.rs

crate/cli/src/commands.rs

crate/client/Cargo.toml

Manuthor · 2025-01-10T14:38:00Z

crate/client/src/config.rs

has to be removed

do we ? why ?

Manuthor · 2025-01-10T14:42:17Z

Cargo.toml

@@ -63,3 +62,5 @@ url = "2.5.4"
 x509-parser = "0.16"
 zeroize = { version = "1.8", default-features = false }
 uuid = { version = "1.11", features = ["v4", "serde"] }
+cosmian_findex = { git = "https://github.com/Cosmian/findex", branch = "feat/update_for_findex_server" }
+cosmian_crypto_core = { version = "9.3.0", default-features = false }


is this dep required?

Suggested change

cosmian_crypto_core = { version = "9.3.0", default-features = false }

Yes, used here in particular crate/structs/src/findex_serialization.rs

HatemMn

Partial response, the rest coming soon

So far looking good for tests except for the huge dataset

crate/cli/src/commands.rs

HatemMn · 2025-01-14T16:35:50Z

crate/cli/src/commands.rs

@@ -88,14 +88,14 @@ impl CoreFindexActions {
    #[allow(clippy::future_not_send)]


I do not agree with this lint rule, at least in this context I don't feel it has meaning

The error :

error: passing a unit value to a function --> crate/cli/src/commands.rs:91:39 | 91 | Self::Datasets(action) => Ok(println!("{}", action.run(findex_client).await?)), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg = note: `#[deny(clippy::unit_arg)]` implied by `#[deny(clippy::complexity)]` help: remove the semicolon from the last statement in the block --> /home/cosmian/.rustup/toolchains/1.83.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/macros.rs:143:9 | 143| $crate::io::_print($crate::format_args_nl!($($arg)*)) | help: or move the expression in front of the call and replace it with the unit literal `()` | 91 ~ Self::Datasets(action) => { 92 + { 93 + $crate::io::_print($crate::format_args_nl!($($arg)*)); 94 + }; 95 + Ok(println!("{}", action.run(findex_client).await?)) 96 ~ },

The fix clippy wants to see :

Self::Datasets(action) => { let result = action.run(findex_client).await?; println!("{}", result); Ok(result) }

Repeating this multiple times, huge boilerplate, etc ...

If you agree with disabling this, mark this as resolved

crate/cli/src/actions/findex/index_or_delete.rs

Cargo.toml

crate/cli/src/actions/findex/search.rs

HatemMn · 2025-01-25T22:24:45Z

crate/cli/src/tests/findex/tests.rs

            findex_parameters: FindexParameters {
-                key: "11223344556677889900AABBCCDDEEFF".to_owned(),
-                label: "My Findex label".to_owned(),
+                key: "11223344556677889900AABBCCDDEEFF11223344556677889900AABBCCDDEEFF".to_owned(),


It's not the same (for now) but it's supposed to be the same

HatemMn · 2025-01-25T22:26:56Z