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

Fields with OfflineFirst() are not updated with the main document when fetching from the server #371

Open
hortigado opened this issue Feb 23, 2024 · 8 comments

Comments

@hortigado
Copy link

Hello again, I have run into a problem that I don't know if it is due to bad practice on my part. But when I request a document with await remote, the document obtains the latest from the server but its OfflineFirst() fields are not obtained from the server but from the local one.

@hortigado hortigado changed the title Fields with Offline First() are not updated with the main document when fetching from the server Fields with OfflineFirst() are not updated with the main document when fetching from the server Feb 23, 2024
@hortigado
Copy link
Author

hortigado commented Feb 24, 2024

I started to review the package and found that indeed, although the parent document comes from FromRest, the association always obtains it from the local. So I had to modify the function. The adapter must also be modified so that the association sends the seedOnly parameter when obtained from FromRest

offline_first_repository.dart

  Future<List<TModel>?> getAssociation<TModel extends RepositoryModel>({required Query query, bool seedOnly = false}) async {
    logger.finest('#getAssociation: $TModel $query');
    final results = await get<TModel>(
      query: query,
      seedOnly: seedOnly,
      policy: seedOnly ? OfflineFirstGetPolicy.awaitRemote : OfflineFirstGetPolicy.awaitRemoteWhenNoneExist,
    );
    if (results.isEmpty) return null;
    return results;
  }

horse_adapter.dart REST

Future<Horse> _$HorseFromSqlite(
  Map<String, dynamic> data, {
  required SqliteProvider provider,
  OfflineFirstWithRestRepository? repository,
}) async {
  return Horse(
    name: data['name'] == null ? null : data['name'] as String?,
    mounties: (await provider.rawQuery(
      'SELECT DISTINCT `f_Mounty_brick_id` FROM `_brick_Horse_mounties` WHERE l_Horse_brick_id = ?',
      [data['_brick_id'] as int],
    ).then((results) {
      final ids = results.map((r) => r['f_Mounty_brick_id']);
      return Future.wait<Mounty>(
        ids.map(
          (primaryKey) => repository!
              .getAssociation<Mounty>(
                query: Query.where('primaryKey', primaryKey, limit1: true),
                seedOnly: true,
              )
              .then((r) => r!.first),
        ),
      );
    }))
        .toList(),
  )..primaryKey = data['_brick_id'] as int;
}

@tshedor
Copy link
Collaborator

tshedor commented Feb 24, 2024

@hortigado Sorry, I'm unclear on the problem and the goal. This is how I'm interpreting both:

Goal:
When I use OfflineFirstGetPolicy.awaitRemote, the instance(s) I'm querying should be hydrated as well as the associations of each instance.

(Defining instance: instance == Horse() and/or instance == repository().get<Horse>().first. Instance is a single version of the model queried)

Problem:
The associations are not awaited. Instead, the queried instance is awaited but the association is always pulled from the local cache.


Is this the correct interpretation? If so, I'd be surprised if seedOnly resolves the problem. Stranger things have happened in Brick, but seedOnly should be completely avoiding any association hydration. That property is designed for eager loading when you don't need to fetch the associations to your local cache because they exist or will exist soon.

@hortigado
Copy link
Author

Hello @tshedor , the problem is that I don't speak English very well, which makes it difficult for me to explain.
But for example I have a Customer document with an association of pizzas. When I get the customer you automatically get the pizzas first from the remote because it does not exist locally. But when you want to update those remote associations it is not possible. since the getAssociation method works with the awaitRemoteWhenNoneExist policy which would never update the remote associations.
The solution I propose is that when we request the parent document from remote, it also obtains the associations from remote by passing the seedOnly=true parameter. When true I apply policy: seedOnly ? OfflineFirstGetPolicy.awaitRemote : OfflineFirstGetPolicy.awaitRemoteWhenNoneExist, so that it waits from remote and does not get from local if it already exists.

image

@hortigado
Copy link
Author

Here you can see that when the policy is not passed it defaults to OfflineFirstGetPolicy policy = OfflineFirstGetPolicy.awaitRemoteWhenNoneExist,
so the solution would be to pass this parameter from the getAssociation method

image

@hortigado
Copy link
Author

hortigado commented Feb 24, 2024

I was also thinking of another solution that the @OfflineFirst(where:) would pass a parameter like hydrate=true so that only the associations that the user needs would be updated each time awaitRemote was called in the parent document.

@tshedor
Copy link
Collaborator

tshedor commented Feb 24, 2024

Hello @tshedor , the problem is that I don't speak English very well, which makes it difficult for me to explain.

Your English is great, and I really respect you for writing complex code in a second language. I wanted to make sure we were on the same page before going on a tangent.

so the solution would be to pass this parameter from the getAssociation method

I agree that the policy should be passed on getAssociation. I think that's the most natural use. If you are explicitly declaring the policy, you would expect it all the way through the associations. Extra parameters could be added, but then they have to be maintained and may clutter the ease-of-use.

Let me write an integration test for this and see if passing the policy through has any side effects.

@hortigado
Copy link
Author

Thank you, it is correct to pass the policy to getAssociation, I think it is the most correct. I did some testing and it works fine. But you know the package better than anyone to make the best decision.
If you want to go deeper, another solution is the one I mentioned, passing this policy or parameter in the @OfflineFirst(where:) for users who only need to hydrate certain associations since each app has different functionalities, although I don't see much sense in the latter.

@tshedor
Copy link
Collaborator

tshedor commented Feb 24, 2024

If you want to go deeper, another solution is the one I mentioned, passing this policy or parameter in the @offlinefirst(where:) for users who only need to hydrate certain associations since each app has different functionalities, although I don't see much sense in the latte

I like where you're going with this, but I worry that this becomes less granular. Setting the option before compile would mean that you couldn't change it during runtime, and you could only make one kind of query in your application code.


I realize why I haven't done this sooner. getAssociation is only invoked in provider serialize/deserialize adapter methods but it's a repository invocation so that associations can be retrieved from the cache. Passing the policy through the adapter method would mean a provider would be aware of repository implementation (i.e. the OfflineFirstGetPolicy enum). Which is bad and avoided throughout Brick. But it's already aware of some repository configuration based on getAssociation's availability. This is poorly designed architecture.

I'd like to add something like the repository abstraction in the adapter method, but covariant doesn't work for enums:

Future<Customer> _$CustomerFromSqlite(Map<String, dynamic> data,
    {required SqliteProvider provider, OfflineFirstWithRestRepository? repository, covariant Policy?}) async {

The other option would be to add it to data like how SQLite does it (..primaryKey = data['_brick_id'] as int). But again, this is a provider-level addition. The provider shouldn't be aware of the repository's implementation.

The final option would be adding it as a stateful field on repository, set on every get request. This would be passed to getAssociation within the repository itself and would never break encapsulation. This could cause the repository to be prone to race conditions. Then again, the repository is already somewhat stateful based on its subscriptions field. I don't love this idea but I don't hate it either.

The easiest thing is to just add an optional parameter to OfflineFirstRepository#getAssociation to permit custom generators to utilize the method. That would band-aid fix this, but it wouldn't solve the unpredictable behavior (this would be the equivalent of your seedOnly addition). This would be required for the stateful repository approach anyway.

@mateominato I know I'm dropping you into a long discussion but I'd appreciate your opinion on this problem. In short, the idea is to add a protected OfflineFirstGetPolicy? latestGetRequestPolicy field to OfflineFirstRepository. This field is set on every #get invocation and removed when the method completes in a finally block. It's accessed in #getAssociation to avoid passing repository configuration to provider adapter methods.

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

No branches or pull requests

2 participants