Refactor streaming_search_inner to return a Stream object #15

keaz · 2024-10-09T17:48:17Z

This commit refactors the streaming_search_inner method in the LdapClient struct to return a Stream object instead of a vector of SearchEntry objects. The Stream object encapsulates the search stream and provides methods for iterating over the search results. This change improves the usability and flexibility of the streaming_search method.

This commit refactors the `streaming_search_inner` method in the `LdapClient` struct to return a `Stream` object instead of a vector of `SearchEntry` objects. The `Stream` object encapsulates the search stream and provides methods for iterating over the search results. This change improves the usability and flexibility of the `streaming_search` method.

StarlessNights

Happy to review!

StarlessNights · 2024-10-10T07:06:04Z

Cargo.toml

Changing the return type of a function warrants a major version bump.

PS. cargo-semver-checks is a pretty neat semver linter.

bumped major version

StarlessNights · 2024-10-10T07:24:26Z

src/lib.rs

@@ -1225,6 +1113,114 @@ impl LdapClient {
    }
 }

+pub struct Stream<'a, S, A> {


This is still only a "stream like object" (just like in ldap3). We want to return an object that implements the futures::stream::Stream trait. I confess that I haven't ever implemented this myself, but taking inspiration from page_turner::mt::PageTurner::pages_ahead it would seem like futures::stream::try_unfold would be able to do most of the work. Probably you can more or less just wrap the code you just wrote inside that.

What's the point you may ask? With Stream implemented we can then bring in StreamExt trait which allows us to use map, filter etc all the iterator like stuff on the stream.

@StarlessNights ,Implemented Stream trait for Stream.

Nice, going the hardcore way I see.

One more thing sprung to my mind. The stream object may be dropped at any point. We should implement Drop and try to abandon the search there, like you've done in next_inner. I suspect that it will be slightly awkward as it's not an async function, but perhaps something like async-dropper could help there.

It isn't easy to implement async-dropper as it expects Eq to be implemented on the Stream.

Yeah hopefully we'll get language support at some point. Perhaps just blocking for the abandoning would be an acceptable compromise. Honestly I don't know how angry the LDAP/AD servers get if you just leave a search dangling?

StarlessNights · 2024-10-14T09:22:11Z

Thanks, I'll take a closer look in the upcoming days.

StarlessNights

Nice work! With a few tweaks we're there I think.

StarlessNights · 2024-10-16T07:56:33Z

src/lib.rs

+                    return Poll::Ready(None);
+                }
+                // Ensure the task is woken when the next record is ready
+                cx.waker().wake_by_ref();


I don't think we should wake here by ourselves. The inner future will do the waking when it's ready. It has the same context.

Yes you are correct.

StarlessNights · 2024-10-16T08:00:49Z

src/lib.rs

@@ -450,7 +455,7 @@ impl LdapClient {
        base: &'a str,
        scope: Scope,
        filter: &'a impl Filter,
-        limit: i32,
+        limit: usize,


limit is a spurious argument. The caller can achieve the same by calling take(n) on the stream.

On the other hand perhaps allowing the user to customize the page-size would be useful here? This would imply calling streaming_search_with internally. See this example in ldap3.

removed the limit and added a separate method for streaming_search_with

StarlessNights · 2024-10-16T08:07:13Z

src/lib.rs

@@ -1225,6 +1113,114 @@ impl LdapClient {
    }
 }

+pub struct Stream<'a, S, A> {


Nice, going the hardcore way I see.

One more thing sprung to my mind. The stream object may be dropped at any point. We should implement Drop and try to abandon the search there, like you've done in next_inner. I suspect that it will be slightly awkward as it's not an async function, but perhaps something like async-dropper could help there.

StarlessNights · 2024-10-16T09:24:11Z

src/lib.rs

+
+impl Record {
+    pub fn to_record<T: for<'b> serde::Deserialize<'b>>(self) -> Result<T, Error> {
+        let json = LdapClient::create_json_signle_value(self.search_entry).unwrap();


Not related to streams anymore but this intermediate conversion to JSON seems unnecessary? serde_json could probably be refactored out completely, but perhaps that's a matter for another PR.

Yes, this is something that I want to improve. I will work on this in a separate PR.

StarlessNights · 2024-10-21T07:48:03Z

src/lib.rs

            .await?;

        Ok(entry)
    }

+    ///
+    /// This method is used to search multiple records from the LDAP server and results will be pageinated.


A small typo here. paginated

StarlessNights · 2024-10-21T07:52:28Z

src/lib.rs

+        let search_stream = self
+            .ldap
+            .streaming_search_with(
+                PagedResults::new(page_size),


EntriesOnly adapter is needed here too according to this comment in ldap3.

StarlessNights · 2024-10-21T08:04:59Z

src/lib.rs

@@ -1225,6 +1113,114 @@ impl LdapClient {
    }
 }

+pub struct Stream<'a, S, A> {


Yeah hopefully we'll get language support at some point. Perhaps just blocking for the abandoning would be an acceptable compromise. Honestly I don't know how angry the LDAP/AD servers get if you just leave a search dangling?

StarlessNights · 2024-10-21T08:08:08Z

src/lib.rs

@@ -1195,6 +1254,13 @@ where
            StreamResult::Finished => Ok(StreamResult::Finished),
        }
    }
+
+    pub async fn cleanup(&mut self) {


...Or manual cleanup isn't so bad either, as long as it's documented.

Documented the cleanup steps.

The print line got left in there. Probably it was just for debugging?

We probably shouldn't abandon searches that have been run to completion. How about checking the stream state and abandoning only on non-finished states?

The unwrap is kinda scary too. Perhaps just return a result?

StarlessNights · 2024-10-22T09:40:59Z

src/lib.rs

+    ///             }
+    ///         }
+    ///     }
+    ///     stream.cleanup().await;
    /// }
    /// ```
    pub async fn streaming_search_with<'a>(


Perhaps something like steaming_search_paged would be a better name, as the caller is not allowed to directly specify the adapters.

Also, do we really need to consume self here?

I guess that the consuming makes some sense given that we're using a pool 🤔.

How about the naming though @keaz?

StarlessNights · 2024-10-22T09:44:36Z

src/lib.rs

@@ -1195,6 +1254,13 @@ where
            StreamResult::Finished => Ok(StreamResult::Finished),
        }
    }
+
+    pub async fn cleanup(&mut self) {


The print line got left in there. Probably it was just for debugging?

We probably shouldn't abandon searches that have been run to completion. How about checking the stream state and abandoning only on non-finished states?

The unwrap is kinda scary too. Perhaps just return a result?

StarlessNights · 2024-10-22T09:53:02Z

README.md

@@ -159,7 +159,7 @@ async fn main() -> Result<()> {
            }
        }
    assert!(count == 2);
-    Ok(ldap.unbind().await?)
+    Ok(result.cleanup()?)


This should still be awaited. (Remember to take into the account the potential change to cleanups type.)

Improved README

StarlessNights · 2024-10-23T07:07:47Z

The CI tests are failing?

keaz · 2024-10-23T17:46:32Z

The CI tests are failing?

Yes, it never worked. I fixed it now.

Removed pull request from ci.yml

StarlessNights

I have zero experience on the github ci stuff so I cannot comment on that.

On the rust side of things all seems good. (Apart from the slightly confusingly named streaming_search_with)

keaz self-assigned this Oct 9, 2024

keaz linked an issue Oct 9, 2024 that may be closed by this pull request

Streaming search that returns a stream #14

Closed

Bump version to 1.7.0 in Cargo.toml

94d41db

keaz mentioned this pull request Oct 9, 2024

Streaming search that returns a stream #14

Closed

updated the README

931de5d

StarlessNights suggested changes Oct 10, 2024

View reviewed changes

keaz added 2 commits October 11, 2024 23:30

Implemented stream for stream_search

d881589

Updated ReadME

8175f35

keaz requested a review from StarlessNights October 14, 2024 08:50

StarlessNights suggested changes Oct 16, 2024

View reviewed changes

Incoperated review comments

66cdc07

StarlessNights suggested changes Oct 21, 2024

View reviewed changes

keaz added 2 commits October 21, 2024 21:28

Addressed review comments

c7961cd

Updated ReadME

c9683e1

keaz requested a review from StarlessNights October 21, 2024 16:04

StarlessNights suggested changes Oct 22, 2024

View reviewed changes

Handled errors in cleanup methos

33a6b3e

Improved README

keaz added 2 commits October 23, 2024 23:09

Fixed CI

d84c665

Fixed CI

fc4a233

keaz added the enhancement New feature or request label Oct 24, 2024

Added Github CI status to README

908709b

keaz requested a review from StarlessNights October 25, 2024 01:00

Update ci.yml

20bda5d

Removed pull request from ci.yml

StarlessNights approved these changes Oct 29, 2024

View reviewed changes

keaz merged commit 839ea08 into main Oct 29, 2024
1 check passed

keaz deleted the streaming-search-that-returns-a-stream branch October 29, 2024 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor streaming_search_inner to return a Stream object #15

Refactor streaming_search_inner to return a Stream object #15

keaz commented Oct 9, 2024

StarlessNights left a comment

StarlessNights Oct 10, 2024

keaz Oct 11, 2024

StarlessNights Oct 10, 2024

keaz Oct 11, 2024 •

edited

Loading

StarlessNights Oct 16, 2024

keaz Oct 17, 2024

StarlessNights Oct 21, 2024

StarlessNights commented Oct 14, 2024

StarlessNights left a comment

StarlessNights Oct 16, 2024

keaz Oct 17, 2024

StarlessNights Oct 16, 2024

keaz Oct 17, 2024

StarlessNights Oct 16, 2024

StarlessNights Oct 16, 2024

keaz Oct 17, 2024

StarlessNights Oct 21, 2024

keaz Oct 21, 2024

StarlessNights Oct 21, 2024

keaz Oct 21, 2024

StarlessNights Oct 21, 2024

StarlessNights Oct 21, 2024

keaz Oct 21, 2024

StarlessNights Oct 22, 2024

StarlessNights Oct 22, 2024

StarlessNights Oct 23, 2024

StarlessNights Oct 22, 2024

StarlessNights Oct 22, 2024

StarlessNights commented Oct 23, 2024

keaz commented Oct 23, 2024 •

edited

Loading

StarlessNights left a comment

Refactor streaming_search_inner to return a Stream object #15

Refactor streaming_search_inner to return a Stream object #15

Conversation

keaz commented Oct 9, 2024

StarlessNights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keaz Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarlessNights commented Oct 14, 2024

StarlessNights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarlessNights commented Oct 23, 2024

keaz commented Oct 23, 2024 • edited Loading

StarlessNights left a comment

Choose a reason for hiding this comment

keaz Oct 11, 2024 •

edited

Loading

keaz commented Oct 23, 2024 •

edited

Loading