furkandeveloper / EasyRepository.EFCore

This repo provides implementation generic repository pattern and specification pattern on Entity Framework Core
MIT License
95 stars 14 forks source link

✨ [FEATURE] Pagination / filtering re-implementation #21

Open jeffward01 opened 2 years ago

jeffward01 commented 2 years ago

Summary

I was working with Pagination, and I found the way it was currently implemented to be restricting. I found (1) bug, and (1) major improvement that could be made.

For my own projects, this is a reason why I would not use this library (no offense). Many people need pagination to return:

Standard Pagination Response Object: https://stackoverflow.com/questions/12168624/pagination-response-payload-from-a-restful-api

{
  "_metadata": 
  {
      "page": 5,
      "per_page": 20,
      "page_count": 20,
      "total_count": 521,
      "Links": [
        {"self": "/products?page=5&per_page=20"},
        {"first": "/products?page=0&per_page=20"},
        {"previous": "/products?page=4&per_page=20"},
        {"next": "/products?page=6&per_page=20"},
        {"last": "/products?page=26&per_page=20"},
      ]
  },
  "records": [
    {
      "id": 1,
      "name": "Widget #1",
      "uri": "/products/1"
    },
    {
      "id": 2,
      "name": "Widget #2",
      "uri": "/products/2"
    },
    {
      "id": 3,
      "name": "Widget #3",
      "uri": "/products/3"
    }
  ]
}

Something like this^^

The key is to get this back as a JSON response object:

 {
      "page": 5,
      "per_page": 20,
      "page_count": 20,
      "total_count": 521,
      "Links": [
        {"self": "/products?page=5&per_page=20"},
        {"first": "/products?page=0&per_page=20"},
        {"previous": "/products?page=4&per_page=20"},
        {"next": "/products?page=6&per_page=20"},
        {"last": "/products?page=26&per_page=20"},
      ]
  }

or at the minimum return this as a JSON response object:

 {
      "page": 5,
      "per_page": 20,
      "page_count": 20,
      "total_count": 521,      
  }

Current Pagination Bugs

    public List<TEntity> GetMultiple<TEntity, TFilter>(EfTrackingOptions asNoTracking, TFilter filter)
        where TEntity : class
        where TFilter : FilterBase
    {
        return this.FindQueryable<TEntity>(asNoTracking)
            .ApplyFilter(filter)
            .ToList();
    }

What is currently implemented (This is the bug):

Proposed Changes

I propose (2) things:

  1. Remove the filtering and pagination from the current library, create a new extension library called EasyRepository.EfCore.AutoFilterer -- This will allow us to implement new filter implementation using Gridify, Sieve and others, instead of currently we are 'married' to AutoFilterer

This is the improvement I mentioned in the beginning of my post.

  1. On Filtered and Paginated Methods, return the TFilter object instead of the TEntity Object. This will be a 'breaking change' as it involves changing what currently implemented methods return.

Closing Statement

To be honest, I need these methods in my own implementation, so I will get started on this. If you reject this proposal I 100% understand, and I will keep the code to myself.

If you think this is helpful and useful, I will add my code and create a pull-request so that our branches can be 'in-sync'.

What are your thoughts? Please make any changes or suggestions as you see fit.

jeffward01 commented 2 years ago

If you'd like to review my code as I am working on it (currently it is complete), here is the branch on my github repo:

https://github.com/jeffward01/EasyRepository.EFCore/tree/explicit-enums-pagination


I am going to do my best to not introduce any additional packages. There is a chance that the Microsoft.DI framework will not be sufficient, I will need to do more investigation on this to confirm.

If it is not sufficient, I will need to add either:

jeffward01 commented 2 years ago

Question for you:

Breaking Change:

There will be a breaking change where the user will no longer have a dependency on AutoFilterer,

The user will need to perform (2) actions to fix the broken changes

  1. The user will need to install their Pagination Library or Filter Library of choice, by using going to nuget.org and installing one of:

    • EasyRepository.EFCore.AutoFilterer
    • EasyRepository.EFCore.Gridify
    • EasyRepository.EFCore.Sieve
      • (I don't plan to implement Sieve at first, this will be later. Gridify is more performant, and has all the primary features that Sieve has... But Sieve is very popular, so I should definitely implement this at some point in the near future)
  2. Then the user will need to go to the container and add this in the DI Registration:

Old Version:


public void ConfigureServices(IServiceCollection services)
{
    // Old Version
    services.ApplyEasyRepository<TDbContext>()

    // Overload...
    services.ApplyEasyRepository<TDbContext>(ServiceLifetime.Scoped);
}

New Version:


public void ConfigureServices(IServiceCollection services)
{
    // New Version - AutoFilterer || Example of Overload is below:
    services.ApplyEasyRepository<TDbContext>().ApplyEasyRepositoryAutoFilterer();
    services.ApplyEasyRepository<TDbContext>(ServiceLifetime.Scoped).ApplyEasyRepositoryAutoFilterer();

     // New Version - Gridify || Example of Overload is below:
    services.ApplyEasyRepository<TDbContext>().ApplyEasyRepositoryGridify();
    services.ApplyEasyRepository<TDbContext>(ServiceLifetime.Scoped).ApplyEasyRepositoryGridify();

    // New Version - Sieve || Example of Overload is below:
    services.ApplyEasyRepository<TDbContext>().ApplyEasyRepositorySieve();
    services.ApplyEasyRepository<TDbContext>(ServiceLifetime.Scoped).ApplyEasyRepositorySieve();
}

// Note: Currently, I plan to throw an exception if  => 2 or more of these filters / paginators are registered.  
// Possible Feature Request: 
// Make more flexible and add configuration so the user can register (2) or more of these filters

Note: Currently, I plan to throw an exception if => 2 or more of these filters / paginators are registered.

jeffward01 commented 2 years ago

Sorry for so many comments, I wanted to make all my requests separate comments for reference ease

Question

I will also update the Wiki. How should we do this?

A.) You grant me access to the Wiki, then I update it B.) I create a new repository, I push the new wiki to it, then you copy and paste the .md and bring it into the Wiki.

I think A is easier, Im not sure if you can grant me 'contributor` only rights with approval, im not sure how the Wiki permissions / roles work on GitHub


Note: I can open up a new issue for this if you want, I was not sure where to put this request...

furkandeveloper commented 2 years ago

It's a nice long topic. We have a static connection with AutoFilterer. Different filtering and pagination libraries can be supported. This should be very well planned and existing users should not be affected by it.

I can understand the response object you want for your projects.

On the link you shared, a reference was made to the pagination response object that the clients should see. AutoFilterer does not have to provide this response object. You can edit the Response object according to your needs.

I understand your requests on this matter, but this is not a feature that we will support right away.

You can fork and make any changes you want for your own projects.

I will keep this request and we will evaluate and develop it together with my team. Thanks for your contribution.

jeffward01 commented 2 years ago

This should be very well planned and existing users should not be affected by it.

In spirit of this, what about if I make the changes as outlined, with the difference of:


    // Note: I'm not proposing this literally, this is just a sketch to convey what I mean
    services.ApplyEasyRepository<TDbContext>(configOptions => {

        configOptions.Pagination = SomeEnum.AutoFilterer

})

Something like this?

You can fork and make any changes you want for your own projects.

Thank you, I will do so.

If it's alright with you, I'd rather keep this discussion open and make the changes for my own projects in-line with your vision and scope, I would massively prefer to make the changes with your vision and scope, rather than with an un-guided vision and scope. If the changes are un-guided, in this sense, the likely-hood of a simple later pull-request being incorporated into the library is much lower as it would require a re-write. I would prefer to avoid re-writes and build it with your vision and scope the first time.


On that note, do you think you can outline some hard requirements for this, and I can get a Scope of Work / Development Plans created that we can discuss? Thank you again for your input and I appreciate your opinion ❤️

I agree with you - I dislike breaking changes and its best to avoid them

jeffward01 commented 2 years ago

I will keep this request and we will evaluate and develop it together with my team. Thanks for your contribution.

Ah, I just saw this bit about the team - Fair enough!

Let me know if I could be of further assistance by tagging me in an issue.

I will stand-down on the Pagination and Filter topic and wait for you and your team

furkandeveloper commented 2 years ago

We have created a plan regarding this issue.

AutoFilterer dependency will be moved to a library named EasyRepository.EFCore.AutoFilterer by applying correct abstraction techniques in future versions.

While creating this library, a document will be created for the integration of new libraries and developers will be able to add new libraries by reading this document.

AutoFilterer is a great library developed by @enisn that generates automatic queries. With the improvements we will make, the AutoFilterer library will be added. We will ask you for help by opening an issue for other libraries. See you in the future my friend. :D