Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagination with nested document returns same new data for both fetchMoreResultData and previousResultData #386

Closed
serendipity1004 opened this issue Aug 20, 2019 · 25 comments

Comments

@serendipity1004
Copy link
Collaborator

I currently have settings like below

void main() {
  final HttpLink httpLink = HttpLink(uri: 'http://cast.mvp.com/apollo_api');

  final AuthLink authLink =
      AuthLink(getToken: () async => await Storage.getAuthToken());

  final Link link = authLink.concat(httpLink);

  ValueNotifier<GraphQLClient> client = ValueNotifier(
    GraphQLClient(
      cache: InMemoryCache(),
      link: link,
    ),
  );

  return runApp(
    GraphQLProvider(
      client: client,
      child: CacheProvider(
        child: MyApp(),
      ),
    ),
  );
}

and I am updating cache like below

update: (Cache cache, QueryResult result) async {
                              if (result.hasErrors) {
                                //TODO error handling
                              } else {
                                final Map<String, Object> mapified =
                                    result.data['votePostComment']
                                        as Map<String, Object>;

                                final String key =
                                    '${mapified['__typename']}/${mapified['id']}';

                                cache.write(key, mapified);

                                refetch();
                              }
                              return cache;
                            },

Cache is getting updated very nicely; however, the change is just not reflected on the screen. I am currently using this under statelesswidget.

Now, when I change the cache to NormalizedCache or OptimisticCache, I can get the changes to be reflected on the screen; however, I encounter another problem where I just cannot get the previousResultData properly (it is always same as the fetchMoreResultData. This, for some reason I have not been able to reproduce so I will try to post more code on it when I can reproduce it. Meanwhile, how can I make InMemoryCache change get reflected throughout the widget enclosed by query?

@mainawycliffe
Copy link
Collaborator

What problem are you facing when getting previousResultsData when using FetchMore?

@serendipity1004
Copy link
Collaborator Author

serendipity1004 commented Aug 20, 2019

@mainawycliffe

HI, now I am able to reproduce the issue.

Flutter test page

import 'package:flutter/material.dart';
import 'package:graphql_flutter/graphql_flutter.dart';

const PAGINATE_TEST_OBJECT = """
  query paginateTestObject(\$next: String){
    paginateTestObject{
      __typename
      id
      nestedObj(next:\$next){
        __typename
        id
      }
    }
  }
""";

class TestScreen1 extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Query(
        options: QueryOptions(
          document: PAGINATE_TEST_OBJECT,
        ),
        builder: (
          QueryResult result, {
          VoidCallback refetch,
          FetchMore fetchMore,
        }) {
          if (result.errors != null) {
            return Text('Err.');
          }

          if (result == null || result.data == null) {
            return Text('loading...');
          }

          final data = result.data['paginateTestObject']['nestedObj'];

          return ListView.builder(
            itemCount: data.length + 1,
            itemBuilder: (context, index) {
              if (index < data.length) {
                return Text(data[index]['id']);
              }

              return RaisedButton(
                child: Text('LOAD MORE'),
                onPressed: () {
                  FetchMoreOptions opts = FetchMoreOptions(
                    variables: {'next': data[data.length - 1]['id']},
                    updateQuery: (previousResultData, fetchMoreResultData) {
                      var oldData = previousResultData['paginateTestObject']['nestedObj'];
                      var newData = fetchMoreResultData['paginateTestObject']['nestedObj'];

                      print(oldData[0]['id']); // THIS ID
                      print(newData[0]['id']); // AND THIS ID IS EXACTLY THE SAME

                      final List<dynamic> combined = [
                        ...oldData as List<dynamic>,
                        ...newData as List<dynamic>
                      ];

                      fetchMoreResultData['paginateTestObject']['nestedObj'] = combined;

                      return fetchMoreResultData;
                    },
                  );

                  fetchMore(opts);
                },
              );
            },
          );
        },
      ),
    );
  }
}

Graphql Server Types

type TestObject{
    id:ID!
    nestedObj(next:String): [NestedObj!]!
}

type NestedObj{
    id:ID!
}

type Query{
    paginateTestObject:TestObject
}

Graphql Server Resolvers

const TestObject = {
    nestedObj: (root: any, params: any, ctx: any) => {
        const arr = new Array(100).fill(0).map((item, index) => {
            return {id:index}
        });

        const {next} = params;

        const body = arr.splice(next ? parseInt(next) + 1 : 0, 20);

        return body;
    }
};

const Query = {
    paginateTestObject: (root:any, params:any, ctx:any) => {
        return {
            id:1
        }
    }
};

export const resolver = {
    TestObject,
    Query
};

My environment

[✓] Flutter (Channel stable, v1.7.8+hotfix.3, on Mac OS X 10.14.5 18F132, locale en-KR)
 
[✓] Android toolchain - develop for Android devices (Android SDK version 28.0.3)
[✓] Xcode - develop for iOS and macOS (Xcode 10.3)
[✓] iOS tools - develop for iOS devices
[✓] Android Studio (version 3.3)
[✓] IntelliJ IDEA Ultimate Edition (version 2018.3.5)
[✓] Connected device (1 available)

• No issues found!

If you run this code, instead of getting new data attached to listview, you loose all the previous data and only get displayed new data.

@serendipity1004
Copy link
Collaborator Author

@mainawycliffe
Also, if you look at the commented prints in the first code, they display exactly the same ids, which I do not believe is correct either.

@mainawycliffe
Copy link
Collaborator

mainawycliffe commented Aug 20, 2019

When you using NormalizedCache and/or OptimisticCache, getting the data is not straightforward as discussed here. You need to cast the data before using it:

final List<dynamic> repos =
          queryResults.data['viewer']['repositories']['nodes'] as List<dynamic>;

@serendipity1004
Copy link
Collaborator Author

@mainawycliffe yea I know and I am using the normalization method as described in the example. Also, as you can see, casting is being implemented correctly.

@serendipity1004
Copy link
Collaborator Author

serendipity1004 commented Aug 21, 2019

@mainawycliffe I can confirm that both previousResultData and fetchMoreData are returning exactly the same data. When you run this, in the first query you get 0 to 19. Then in second you get 20 to 39 + 20 to 39 and next 40 to 69 + 40 to 69 and so on. (duplicated fetchMoreData)

What about just normal InMemoryCache? Is there not a way to make the query reflect the changes made in InMemoryCache?

@serendipity1004 serendipity1004 changed the title How to make sure IneMemoryCache change gets reflected to queries How to make sure InMemoryCache change gets reflected to queries Aug 21, 2019
@serendipity1004 serendipity1004 changed the title How to make sure InMemoryCache change gets reflected to queries Pagination with nested document returns same new data for both fetchMoreResultData and previousResultData Aug 21, 2019
@serendipity1004
Copy link
Collaborator Author

serendipity1004 commented Aug 21, 2019

@mainawycliffe

now I can confirm that this is problem with nested pagination. Pagination without nested object pagination works as expected but nested object pagination returns same data for both previous data and fetched data.

#386 (comment) this is the code that fetches nested pagination data

below is the code that fetches not nested pagination data.

import 'package:flutter/material.dart';
import 'package:graphql_flutter/graphql_flutter.dart';

const PAGINATE_TEST_OBJECT = """
  query paginateTestObject(\$next: String){
    paginateTestObject{
      __typename
      id
      nestedObj(next:\$next){
        __typename
        id
      }
    }
  }
""";

const PAGINATE_TEST_OBJECT2 = """
  query paginateTestObject2(\$next: String){
    paginateTestObject2(next:\$next){
      __typename
      id
    }
  }
""";

class TestScreen1 extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Query(
        options: QueryOptions(
          document: PAGINATE_TEST_OBJECT2,
          fetchPolicy: FetchPolicy.networkOnly
        ),
        builder: (
          QueryResult result, {
          VoidCallback refetch,
          FetchMore fetchMore,
        }) {
          if (result.errors != null) {
            return Text('err');
          }

          if (result == null || result.data == null) {
            return Text('loading...');
          }

          final List data = result.data['paginateTestObject2'];

          return ListView.builder(
            itemCount: data.length + 1,
            itemBuilder: (context, index) {
              if (index < data.length) {
                return Text(data[index]['id'].toString(),);
              }

              return RaisedButton(
                child: Text('LOAD MORE'),
                onPressed: () {
                  FetchMoreOptions opts = FetchMoreOptions(
                    variables: {'next': data[data.length - 1]['id']},
                    updateQuery: (previousResultData, fetchMoreResultData) {
                      var oldData = previousResultData['paginateTestObject2'];
                      var newData = fetchMoreResultData['paginateTestObject2'];

                      final List<dynamic> combined = [
                        ...oldData as List<dynamic>,
                        ...newData as List<dynamic>
                      ];

                      fetchMoreResultData['paginateTestObject2'] = combined;

                      return fetchMoreResultData;
                    },
                  );

                  fetchMore(opts);
                },
              );
            },
          );
        },
      ),
    );
  }
}

Nested object pagination returns same data for both fetchMoreData and previousData ONLY when OptimisticCache or NormalizedInMemoryCache is used. If InMemoryCache is used, there's no problem with returned data. I feel like it's likely to be a problem with example normalization technique. Maybe the data is getting saved by the root object __typename/id and it's causing overwrite of the nested data so previous data and fetched data is the same?

@serendipity1004
Copy link
Collaborator Author

My guess was correct. If I tag the key of the cache with unique values for every iteration, the previousFetchData value does not get overwritten.

We need to implement something like https://www.apollographql.com/docs/react/features/pagination/#the-connection-directive as presented in apollo-graphql.

For those of you having same problem as I am, currently the only way to solve this problem is by providing an additional unique value to the cache key so that the data from fetchMore does not overwrite previousData. If you can point me to right direction, I could try to contribute to this feature.

@mainawycliffe
Copy link
Collaborator

What about just normal InMemoryCache? Is there not a way to make the query reflect the changes made in InMemoryCache?

I am not well familiar with the caching part of this package, I would have to look into it to whether this is possible and how to do it. Maybe @micimize can help.

@mainawycliffe
Copy link
Collaborator

If I understand your problem correctly, it seems due to the nested fields in your query, the normalization key is the same for the subsequent fetch more queries, hence they get overridden in the cache. So, am guessing here maybe you should look at your typenameDataIdFromObject that generates the cache key. If you are using the default one and due to the nested fields nature, I can see why the cache gets overridden since it will resolve using the _typename and id up top and not nested one. I will also look into this, using the Github API which is of a similar nature, when I get some time.

@serendipity1004
Copy link
Collaborator Author

@mainawycliffe yes. The issue is exactly as you say. I have solved this issue by providing random uuid from the server and also including that as the cache key if I am calling nested pagination. But now the problem is that when Query and Mutation are in the same StatelessWidget, calling mutation causes query to re-run. Is this a known and intended behaviour?

@serendipity1004
Copy link
Collaborator Author

@mainawycliffe so say for example, if I make a query that retrieves posts (infinite scroll) and call mutation when likes are clicked on each post, query is run again and the list is reset to index 0 because original query does not provide the cursor, only fetchMore does. I have started another thread here #389 (comment)

@micimize
Copy link
Collaborator

@serendipity1004 This is not intended behavior - we hadn't considered the case where fetchMore returns a partial of an in-cache entity. Very good work isolating the issue.

A temporary workaround would be to fetchMore with

paginateTestObject {
      __typename
      pageId: id
}

That way the entity won't be picked up by the normalizer, and you can merge the results yourself.

The correct solution is probably for us to run the fetchMore updateQuery pre-normalization of the new results, or at least add that as an option

@mainawycliffe
Copy link
Collaborator

mainawycliffe commented Aug 21, 2019

I think it's best to run fetchMore updateQuery method pre-normalization unless there are unforeseen side effects. It would certainly prevent the need for the workaround.

@micimize
Copy link
Collaborator

@serendipity1004 #394 should fix both your issues -You can test by overriding graphql in your pubspec.yaml:

dependency_overrides:
  graphql:
    git:
      url: git://github.com/micimize/graphql-flutter.git
      ref:  fetchmore_patch
      path: packages/graphql

Please give it a shot - I did some testing on #389 but not this issue.

Also, worth noting that your first update is async, which isn't supported and caused the initial issue. So, cache gets updated asynchronously, but the UI update happens synchronously so you don't see the change.

update: (Cache cache, QueryResult result) async {

@serendipity1004
Copy link
Collaborator Author

@micimize I can confirm that it is working on both the example that I have provided and on my previous commit of my project. Great job and thank you. May I ask when this will be merged into the main branch?

@serendipity1004
Copy link
Collaborator Author

serendipity1004 commented Aug 28, 2019

@micimize I have to investigate a little more but there's another problem when using fetchMore. When I am creating an infinite loading list with fetchMore and set the query option to be cacheFirst, I am losing the data fetched with fetchMore query when I move to another route and come back (My data in the ListView is returned to the first ever query with no cursor in the query variable). My first guess is that either the query is not reaching for the cache first OR cache is being reached first but the following query without cursor is completely replacing the cache. I will try to investigate more into the issue and let you know.

@mainawycliffe
Copy link
Collaborator

FetchMore requests are never cached, the cache settings are purposefully ignored. We are working on improvements, tracked on this issue, but FetchMore is a complicated feature, as its uses are huge.

@serendipity1004
Copy link
Collaborator Author

@mainawycliffe fair enough. May I know why cache is purposefully ignored in fetchMore? Just wondering how it could be useful in some cases.

@mainawycliffe
Copy link
Collaborator

The main reason is that it's the expected common use case. I can see the advantages of caching fetch more results, but it also brings up issues highlighted here like replacing the previous results during cache normalization.

@micimize
Copy link
Collaborator

@mainawycliffe no, he's on my fork for #394 so the results of fetchMore's updateQuery should get cached under the key with the initial variables. Will take a look later

@micimize
Copy link
Collaborator

@HofmannZ why do we override the fetch policy if the default query option is used? Why not just change the default?

https://github.com/zino-app/graphql-flutter/blob/633df3eda6c57a38aac6ee58baa4ac3f32630c95/packages/graphql_flutter/lib/src/widgets/query.dart#L41-L43

@mainawycliffe
Copy link
Collaborator

@micimize I saw that and I didn't understand why. Someone already raised the same issue and there is a PR for that.

@micimize
Copy link
Collaborator

Ok, well when I remove that block going back to the fetchmore query has no issues, so I'm leaving that for another (soon-to-be) PR

@micimize
Copy link
Collaborator

closed by #394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants