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

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

Open
jeffward01 opened this issue Oct 1, 2022 · 7 comments
Open

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

jeffward01 opened this issue Oct 1, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@jeffward01
Copy link

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

  • You use the library AutoFilterer to implement filtering and pagination
  • When you work with filtering and pagination, you eliminate any Pagination Object to be returned. See Code below
    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):

  • There is an DBO Entity, just something in the database
  • You apply the pagination style filtering to it
  • Then you return the DBO Entity instead of the Pagination Response Object
  • This means if the user has some pagination properties such as Total Pages, Current Page, Total Results, etc... These will not be returned to the user. Only the DBO Entity will be returned.

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.
  • Perhaps, to avoid the breaking change, instead we add a suffix of Paged or Filtered to the method. I prefer the suffix of Paged as mostly all Filtered use-cases for UI's will also be Paged.

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 jeffward01 added the enhancement New feature or request label Oct 1, 2022
@jeffward01
Copy link
Author

jeffward01 commented Oct 1, 2022

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:

  • Scrutor: Scans assemblies and makes locating injected assemblies and types mostly easy
    or
  • Stashbox: This is a fast easy to use DI framework, similar to AutoFac, but much much faster, about 700% - 2,500% faster than AutoFac (I don't know why people still use Autofac, or even Windsor shudders at the thought of Windsor) - This will be internally used, the consuming developer will have no interactions with it. Simply, it will be used to resolve target implementations of Paginations/Filters

@jeffward01
Copy link
Author

jeffward01 commented Oct 1, 2022

Question for you:

  • If you accept this change, what version should I iterate the package to?

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.

  • Possible Feature Request: Make more flexible and add configuration so the user can register (2) or more of these filters

@jeffward01
Copy link
Author

jeffward01 commented Oct 1, 2022

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
Copy link
Owner

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
Copy link
Author

jeffward01 commented Oct 1, 2022

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:

  • Autofilterer will be included and used by default
  • None of the existing return objects or methods will be updated
  • Additional methods will be added to support other 'paginated' responses using Gridify, Sieve, or even AutoFilterer which can be configured in the startup class by something like:
    // 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
Copy link
Author

jeffward01 commented Oct 1, 2022

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
Copy link
Owner

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

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

No branches or pull requests

2 participants