New data source #105

sunli829 · 2020-05-24T12:14:51Z

I rewrote the code of DatsSource.
The DataSource implemented for Stream has been temporarily removed, and this modification will be added after everyone agrees.

IcanDivideBy0

Some minor remarks, otherwise looks good to me!

src/types/connection/connection_type.rs

src/types/connection/cursor.rs

phated

Bunch of comments and questions.

phated · 2020-05-24T23:22:48Z

src/base.rs

-            return None;
-        }
-        Some(v[1].to_string().into())
+        base64::decode(id.as_str())


This is a weird pattern to force people into. If you structure data properly, this isn't needed because you can do a trip to the database to find the type. You shouldn't be relying on types sent from the client.

I removed Type::global_id and Type::from_global_id. 🙂

phated · 2020-05-24T23:23:42Z

src/types/connection/connection_type.rs

+}
+
+impl<C, E, T> Record<C, E, T> {
+    pub fn new(cursor: C, edge: E, element: T) -> Self {


This is great! 👍 I was going to implement this for my own stuff.

phated · 2020-05-24T23:24:22Z

src/types/connection/connection_type.rs

+}
+
+impl<C, T> Record<C, EmptyEdgeFields, T> {
+    pub fn new_without_edge_fields(cursor: C, element: T) -> Self {


I think we should make new be without EdgeFields and the make this new_with_edge_fields. The right defaults are important.

If we change the name of my suggestion above, we could call this new_with_additional_fields.

phated · 2020-05-24T23:28:36Z

src/types/connection/connection_type.rs

 use indexmap::map::IndexMap;
 use inflector::Inflector;
 use itertools::Itertools;
 use std::borrow::Cow;

+/// The record type output by the data source
+pub struct Record<C, E, T> {


If we want to map to the relay spec, I believe this should be named Edge

phated · 2020-05-24T23:28:59Z

src/types/connection/connection_type.rs

+/// The record type output by the data source
+pub struct Record<C, E, T> {
+    cursor: C,
+    edge: E,


This would then be named additional_fields (or something similar)

phated · 2020-05-24T23:37:01Z

src/types/connection/cursor.rs

-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.0
-    }
+    fn encode_cursor(self) -> String;


I think encoding should be fallible too.

src/types/connection/cursor.rs

phated · 2020-05-24T23:38:46Z

src/types/connection/cursor.rs

-    fn to_json(&self) -> Result<serde_json::Value> {
-        Ok(self.0.to_string().into())
+    fn encode_cursor(self) -> String {
+        base64::encode((self as u32).to_be_bytes())


This should be fallible.

phated · 2020-05-24T23:39:54Z

src/types/connection/page_info.rs

-    #[field(desc = "When paginating backwards, the cursor to continue.")]
-    pub start_cursor: Option<Cursor>,
+    /// When paginating backwards, the cursor to continue.
+    pub start_cursor: Option<String>,


Option<impl CursorType>?

This requires PageInfo<C>, but it's an internal type, so String might be more convenient.

phated · 2020-05-24T23:39:58Z

src/types/connection/page_info.rs

-    #[field(desc = "When paginating forwards, the cursor to continue.")]
-    pub end_cursor: Option<Cursor>,
+    /// When paginating forwards, the cursor to continue.
+    pub end_cursor: Option<String>,


Option<impl CursorType>?

IcanDivideBy0

Just a bit of renaming to makes things clearer

src/types/connection/connection.rs

src/types/connection/edge.rs

phated

This is really looking good. Other than the naming comments from @IcanDivideBy0, I'm giving 👍

Should we make EdgeFieldsType an Option to remove EmptyEdgeFields from the API? Now that we have an API for new_with_additional_fields, users wouldn't need to operate on the Some/None.

sunli829 · 2020-05-26T02:25:03Z

This is really looking good. Other than the naming comments from @IcanDivideBy0, I'm giving 👍

Should we make EdgeFieldsType an Option to remove EmptyEdgeFields from the API? Now that we have an API for new_with_additional_fields, users wouldn't need to operate on the Some/None.

This may not be removable, maybe I have not found a way. 😀

phated · 2020-05-26T03:09:16Z

@sunli829 do you want me to try making EdgeFieldsType an Option?

sunli829 · 2020-05-26T03:20:57Z

@sunli829 do you want me to try making EdgeFieldsType an Option?

Yes, thank you, and I want to remove the totalCount on the Connection and add additional fields to the Connection. 😁

sunli829 · 2020-05-26T03:26:01Z

@sunli829 do you want me to try making EdgeFieldsType an Option?

Yes, thank you, and I want to remove the totalCount on the Connection and add additional fields to the Connection. 😁

I am going to solve this problem now. There are potential security issues here, and I found a way to avoid using unsafe code.

async-graphql/src/subscription/connection.rs

Line 132 in 2bb2c25

// The following code I think is safe.😁

phated · 2020-05-26T03:41:51Z

👍 I will let you work on it! 🍻

sunli829 · 2020-05-27T02:56:20Z

Is it impossible to make EdgeFieldsType into Option? @phated 😁

phated · 2020-05-27T03:01:33Z

I didn't work on it because I thought you were trying. Would you like me to work on it?

sunli829 · 2020-05-27T03:10:17Z

I didn't work on it because I thought you were trying. Would you like me to work on it?

Yes. 😅

sunli829 · 2020-05-27T03:19:32Z

I don't know how to do this. Maybe you can help me with it. 😁

sunli829 · 2020-05-27T03:29:04Z

We may need to remove the generic parameter E on Connection to do this.

phated · 2020-05-27T04:18:50Z

@sunli829 I don't see an easy way to support this without a lot of refactor.

sunli829 · 2020-05-27T04:21:51Z

@sunli829 I don't see an easy way to support this without a lot of refactor.

Only trait objects can be used, which will reduce performance and feel unnecessary.

phated · 2020-05-27T04:23:11Z

I honestly don't understand the benefit of these extra fields, but I just want this feature finished so I can use it.

src/types/connection/edge.rs

phated

@sunli829 this looks GREAT! Let's ship it!

phated · 2020-05-28T03:56:03Z

@sunli829 I added this to my project locally and it is very cool! I'm excited to see @IcanDivideBy0 re-introduce his Stream support

sunli829 · 2020-05-28T04:13:44Z

@sunli829 I added this to my project locally and it is very cool! I'm excited to see @IcanDivideBy0 re-introduce his Stream support

The current implementation does not conflict with StreamDataSource and can be easily added again. @IcanDivideBy0

IcanDivideBy0 · 2020-05-28T05:55:40Z

@phated @sunli829 I'll take a look a this today!

mwilliammyers · 2020-05-28T16:17:38Z

I don’t know how I didn’t catch this before but according to the Rust guidelines for APIs, it should be with_additional_fields not new_with_additional_fields. I know it’s small but, @sunli829, you said you appreciate clean APIs 🙃

Is it worth changing those?

phated · 2020-05-28T20:46:41Z

@mwilliammyers seems good. Want to open an issue? I think we can keep both around for awhile since it doesn't hurt anything.

sunli829 · 2020-05-29T00:26:31Z

At the beginning, with_additional_fields was not provided because it usually seems to be an option, but in fact it is required.

async fn execute_query(
            &self,
            _ctx: &Context<'_>,
            after: Option<usize>,
            before: Option<usize>,
            first: Option<usize>,
            last: Option<usize>,
        ) -> FieldResult<
            Connection<
                usize, // cursor
                u32, // node
                MyConnectionFields,
                EmptyFields,
            >,
        >
{
    Connection::new(...). // Connection<usize, u32, EmptyFields, EmptyFields>
          with_additional_fields(MyConnectionFields{ ... }); // Connection<usize, u32, MyConnectionFields, EmptyFields>
}

If with_additional_fields is missing, it will fail to compile.

sunli829 · 2020-05-29T02:26:56Z

Do you just want to change the name of this function?

phated · 2020-05-29T02:32:04Z

@sunli829 yes it is just to change the name because of Rust guideline recommendation.

new_with_additional_fields ➡️ with_additional_fields

IcanDivideBy0 · 2020-05-29T10:11:43Z

Ok, now that I've spend some hours trying to get thing working for Stream, I'll share here where I've come...

The new CursorType is not really easy to work with, mostly because of the Sync requirement. I've managed to almost completely remove the Sync everywhere from Schema, Resolver, ObjectType impls etc but got stuck at some point with Data. I still think this can be achieved but it's getting too far beyond my comprehension of the crate as a whole :/
(If someone can point me why we cannot remove Sync from almost the whole API, that'd be enlightening)

I think the most easy way would be to remove the CursorType trait which don't really add any value to async_graphql, forcing it as String everywhere it's used and let users encode / decode their Cursor in the execute_query function since it already return a Result. Also It feels a bit strange to return a Connection with the cursor type set to ID and getting a String in the schema for example.

That being said, one change that will be required anyways to implement DataSource for Stream is to change query and execute_query signatues to &mut self

sunli829 · 2020-05-29T11:25:07Z

Let me think for a moment. 😀

sunli829 · 2020-05-29T11:31:19Z

Can you tell me where you use stream datasource？

IcanDivideBy0 · 2020-05-29T12:38:00Z

I was trying to bring back

impl<C, T> DataSource for Pin<Box<dyn Stream<(C, T)>>>
where
    C: CursorType,
    T: OutputValueType

sunli829 · 2020-05-29T12:46:48Z

Do you give the results of the MongoDB query to Stream DataSource?

sunli829 · 2020-05-29T16:15:40Z

Ok, now that I've spend some hours trying to get thing working for Stream, I'll share here where I've come...

The new CursorType is not really easy to work with, mostly because of the Sync requirement. I've managed to almost completely remove the Sync everywhere from Schema, Resolver, ObjectType impls etc but got stuck at some point with Data. I still think this can be achieved but it's getting too far beyond my comprehension of the crate as a whole :/
(If someone can point me why we cannot remove Sync from almost the whole API, that'd be enlightening)

I think the most easy way would be to remove the CursorType trait which don't really add any value to async_graphql, forcing it as String everywhere it's used and let users encode / decode their Cursor in the execute_query function since it already return a Result. Also It feels a bit strange to return a Connection with the cursor type set to ID and getting a String in the schema for example.

That being said, one change that will be required anyways to implement DataSource for Stream is to change query and execute_query signatues to &mut self

A few days ago I also wanted to remove Sync, but I failed. 😂

phated · 2020-05-29T16:21:22Z

I also made my cursor a string. Should we change that?

sunli829 · 2020-05-29T16:23:02Z

I also made my cursor a string. Should we change that?

I think this is not a problem. In most cases, the cursor should be a base64 encoded string.

sunli829 · 2020-05-29T16:25:12Z

In fact, I even want to delete these two.

impl CursorType for String
impl CursorType for ID

I think it is up to users to implement Cursor, not us. 😂

phated · 2020-05-29T16:42:52Z

@sunli829 we need to delete the cursor type. That's what is causing the Sync problems. Please make it only a String

sunli829 · 2020-05-29T16:55:45Z

Cursor is already implemented for the String type. What is the difference between removing Cursor and replacing it with a String？

phated · 2020-05-29T17:27:06Z

I believe @IcanDivideBy0 already explained why it doesn't work but maybe we need to see his code?

IcanDivideBy0 · 2020-05-29T19:08:33Z

Ohhh my bad, I don't know why but I was absolutely convinced that a stream of Sync items have to be Sync... So I went straight up in this direction, trying to remov the Sync requirement on CursorType

So, in the end, this compile fine:

use crate::connection::{Connection, CursorType, DataSource, EmptyFields};
use crate::{Context, FieldResult, OutputValueType};
use async_graphql_derive::DataSource;
use futures::Stream;
use std::pin::Pin;

#[DataSource(internal)]
impl<'a, C, T> DataSource for Pin<Box<dyn Stream<Item = (C, T)> + Send + 'a>> // Not Sync !
where
    C: CursorType + Send + Sync + 'a,
    T: OutputValueType + Send + Sync + 'a,
{
    type CursorType = C;
    type NodeType = T;
    type ConnectionFieldsType = EmptyFields;
    type EdgeFieldsType = EmptyFields;

    async fn execute_query(
        &mut self,
        _ctx: &Context<'_>,
        after: Option<Self::CursorType>,
        before: Option<Self::CursorType>,
        first: Option<usize>,
        last: Option<usize>,
    ) -> FieldResult<
        Connection<
            Self::CursorType,
            Self::NodeType,
            Self::ConnectionFieldsType,
            Self::EdgeFieldsType,
        >,
    > {
        unimplemented!()
    }
}

Still a few gotchas tho

We need to get get a mutable ref to the stream to pull nodes out of it
Previously my first aim was to reduce memory usage by not having to pull a large stream into a vec to slice it right after completion. Since performances were pretty bad on previous implementation, I wonder if we really should provide this implementation in the crate.

phated · 2020-05-29T19:43:23Z

I'm having a hard time conceptualizing how my database stream will be used with the Stream datasource since my return type is a GraphQL Union. The &self reference has been hard for me because I can't implement DataSource against a Rust enum, which we require for the Union macro—which required me to implement a dummy struct just to have a DataSource.

I was hoping to remove that &self reference completely and store data in the Context, but maybe that's the wrong way to think about this?

As for performance, I say we implement it with "bad" performance and then improve the performance once we can start using it in real scenarios. What do you think?

sunli829 · 2020-05-29T21:05:41Z

I think the datasource should be stored in the context and not created at query time, that's why it's Sync and &self.
Query all the data from the database and then filter out most of the records through paging in the stream data source, if my understanding of the stream data source is correct. Then I would never use it, why not use a database query to filter out these records?

New data source

7b4862b

sunli829 requested review from phated and IcanDivideBy0 and removed request for phated May 24, 2020 12:14

IcanDivideBy0 approved these changes May 24, 2020

View reviewed changes

src/types/connection/connection_type.rs Outdated Show resolved Hide resolved

src/types/connection/cursor.rs Show resolved Hide resolved

phated reviewed May 24, 2020

View reviewed changes

Implement Cursor for String and ID

fb02410

sunli829 marked this pull request as draft May 24, 2020 23:54

sunli829 requested review from phated and IcanDivideBy0 May 24, 2020 23:54

sunli829 added 2 commits May 25, 2020 14:12

Remove the Record type and replace it with Edge

5aecf44

Change the return value of Cursor::encode_cursor to Result

d1d3c97

IcanDivideBy0 approved these changes May 25, 2020

View reviewed changes

src/types/connection/connection.rs Outdated Show resolved Hide resolved

src/types/connection/edge.rs Outdated Show resolved Hide resolved

phated reviewed May 25, 2020

View reviewed changes

sunli829 added 2 commits May 26, 2020 10:48

Add docs for datasource

c215c36

Rename connection.rs to connection_type.rs

6e23ca8

IcanDivideBy0 approved these changes May 27, 2020

View reviewed changes

src/types/connection/edge.rs Outdated Show resolved Hide resolved

Rename element to node.

bd3166e

phated approved these changes May 27, 2020

View reviewed changes

phated marked this pull request as ready for review May 27, 2020 16:49

Merge branch 'master' into new-datasource

52db72c

sunli829 merged commit ada2597 into master May 28, 2020

sunli829 deleted the new-datasource branch May 28, 2020 04:14

New data source #105

New data source #105

Conversation

sunli829 commented May 24, 2020

IcanDivideBy0 left a comment

Choose a reason for hiding this comment

phated 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

IcanDivideBy0 left a comment • edited

Choose a reason for hiding this comment

phated left a comment

Choose a reason for hiding this comment

sunli829 commented May 26, 2020

phated commented May 26, 2020

sunli829 commented May 26, 2020

sunli829 commented May 26, 2020

phated commented May 26, 2020

sunli829 commented May 27, 2020

phated commented May 27, 2020

sunli829 commented May 27, 2020

sunli829 commented May 27, 2020

sunli829 commented May 27, 2020

phated commented May 27, 2020

sunli829 commented May 27, 2020

phated commented May 27, 2020

phated left a comment

Choose a reason for hiding this comment

phated commented May 28, 2020

sunli829 commented May 28, 2020

IcanDivideBy0 commented May 28, 2020

mwilliammyers commented May 28, 2020 • edited

phated commented May 28, 2020

sunli829 commented May 29, 2020 • edited

sunli829 commented May 29, 2020

phated commented May 29, 2020 • edited

IcanDivideBy0 commented May 29, 2020

sunli829 commented May 29, 2020

sunli829 commented May 29, 2020

IcanDivideBy0 commented May 29, 2020 • edited

sunli829 commented May 29, 2020

sunli829 commented May 29, 2020

phated commented May 29, 2020

sunli829 commented May 29, 2020

sunli829 commented May 29, 2020 • edited

phated commented May 29, 2020

sunli829 commented May 29, 2020

phated commented May 29, 2020

IcanDivideBy0 commented May 29, 2020

phated commented May 29, 2020

sunli829 commented May 29, 2020 • edited

IcanDivideBy0 left a comment •

edited

mwilliammyers commented May 28, 2020 •

edited

sunli829 commented May 29, 2020 •

edited

phated commented May 29, 2020 •

edited

IcanDivideBy0 commented May 29, 2020 •

edited

sunli829 commented May 29, 2020 •

edited

sunli829 commented May 29, 2020 •

edited