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

Introduce IRepository.DisableTracking() extension method #17487

Closed
hikalkan opened this issue Aug 25, 2023 · 0 comments · Fixed by #17491
Closed

Introduce IRepository.DisableTracking() extension method #17487

hikalkan opened this issue Aug 25, 2023 · 0 comments · Fixed by #17491

Comments

@hikalkan
Copy link
Member

hikalkan commented Aug 25, 2023

Continues from #1848

We've implemented "no tracking by default" for readonly repositories. See #597 and see document: https://github.com/abpframework/abp/blob/fe95e8ddd0d236dc4003d96a1a90cb64f6f7c619/docs/en/Entity-Framework-Core.md#read-only-repositories

However, it doesn't solve everything. Because, most of the time, we are injecting a regular IRepository<...>, then in some method we only need querying, but in some other methods in the same application service, we need to modify objects.

One implementation propasal from me is to introduce bool IRepository.IsChangeTrackingEnabled property. We can set it to enable or disable change tracking for all get operations on this repository object. Usage:

  • Set _repository.IsChangeTrackingEnabled to false.
  • Query from the _repository object
  • Set _repository.IsChangeTrackingEnabled to true only if you need to enable it in the same request again (if your application service method is just returning data, you don't need to re-enable it since the repository object already will be disposed after the request).

As a shortcut, we can introduce a DisableTracking() extension method:

public async Task<List<TodoItemDto>> GetListAsync()
{
    using(_todoItemRepository.DisableTracking())
    {
        var items = await _todoItemRepository.GetListAsync(); //items are not tracked
    }

    var items2 = await _todoItemRepository.GetListAsync(); //items are tracked again
}

If you want to disable tracking for a complete method body, you may not use the using block:

public async Task<List<TodoItemDto>> GetListAsync()
{
    _todoItemRepository.DisableTracking();

    var items = await _todoItemRepository.GetListAsync(); //items are not tracked
}

USAGE WARNING: This approach is safe for typical application service / controller methods. However, if another methods of this service instance is called after GetListAsync, tracking will remain disabled. So, it is a best practice to always use the using statement if you are not sure that only one method of this service is called, then it is disposed. (so we won't mention this usage in docs)

IMPLEMENTATION NOTES

  • When we dispose the object returned from the DisableTracking() method, it should not directly set IsChangeTrackingEnabled to true, but should always set to the previous value.
  • We can also introduce EnableTracking() that works exactly opposite of DisableTracking().
  • It won't be implemented for MongoDB, since it doesn't have such a change tracking system. For MongoDB repositories, we can still use the same API, but it won't have any effect.

Other Ideas

Introducing a DisableEntityChangeTracking attribute would be a more comfortable way (it disables tracking only for a specific method body or can be used on a class to disable for all methods in a class) in many cases:

[DisableEntityChangeTracking]
public async Task<List<TodoItemDto>> GetListAsync()
{
    var items = await _todoItemRepository.GetListAsync(); //items are not tracked
}

That requires to implement ambient context pattern that works independently from repository objects (implemented with interception). So, instead of bool IsChangeTrackingEnabled, we can define nullable bool (bool? IsChangeTrackingEnabled) which is null by default. If it is null, it uses the value in the ambient context. If it is also null, goes with default behavior.

EnableEntityChangeTracking attribute can also be nice to convert the behaviour vice verse (assume that a method is called from another method that disables tracking, but we want to enable tracking for the current method).

We could even define a convention that change tracking is disable for the application service / controller method starting with Get prefix in naming, but this will be a great breaking change. Even if we do it optional, it can be dangerous. So, change tracking should be disabled carefully, only if a performance difference occurs. For example, for a single entity querying methods, it doesn't worth to disable change tracking.

Even other ideas

This feature has some overlaping with the #1848
Because making a repository read-only automatically disables change tracking. It can be better to discard IsReadOnly property, and set IsChangeTrackingEnabled to false for read-only repositories. One drawback is that we can't throw exception if we call UpdateAsync method on a readonly repository (which is only possible if we cast it to IRepository<...>). But I think we can ignore that problem and cancel throwing exceptions in these cases. Agree?

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

Successfully merging a pull request may close this issue.

2 participants