vapor-community / pagination

Simple Vapor 3 Pagination
MIT License
64 stars 15 forks source link

Feature: Advanced queries #21

Closed mikkelu closed 5 years ago

mikkelu commented 5 years ago

First of all, thanks for this great package. I have been using it a lot. However, I ran into some difficulties when trying to do more advanced stuff like custom response types and advanced queries, e.g., joins, also decode, etc.

I want to apologize for this PR becoming so massive, but I created this for our own API's, and I ended up writing a lot on top of the current implementation to allow for customizations. I did not have time to break this into smaller chunks, and this PR would probably require the package to do a bump in major.

This implementation is tested and works in production, but I think we should use this PR to discuss how a Pagination 2.0.0 should look. Nonetheless, here are the changes:

  1. My first issue was not being able to do advanced queries such as joins and transforming the data before being created as a paginated response. To accomplish this, I have changed the page generic to only require conformance to Content. Next, I could introduce a transformation closure in the page functions: transform: @escaping (QueryBuilder) throws -> Future<[R]> which now allows us to transform the underlying query builder to an actual result and hence transform the result. This allows us to do this (which solves #11 #12 ):
    
    Galaxy.query(on: req).paginate(on: req) { builder in
    builder
        .join(\Planet.galaxyID, to: \Galaxy.id)
        .alsoDecode(Planet.self)
        .all()
        .map { $0.map { GalaxyPlanetResponse(gal: $0.0, plan: $0.1) }}
    }

( -> Future<Paginated> )



2. Next issue was that of creating a custom paginated response. Instead of only allowing `Paginated` response object, I created a protocol (`PaginatedResponse`) that requires you to implement an initializer that takes a `Page<Content>` as input to create the paginated response: 
`init(from page: Page<M>)`
which now allows everyone to create their custom responses, just by implementing this protocol. This also solves #14 (a solution to the question can be found in the tests). 

3. All new implementations have been created with tests to verify it works. I have also created new tests that mock a real router, so it's more close to production use cases. I also did some minor cleaning, documenting and linting. 

All in all, this should allow people to do more customized stuff with this package. Also, this does not break any current implementations. However, I still think this would require a major bump. 

Let me know what you guys think - and If this PR is accepted, I will update the docs. 
anthonycastelli commented 5 years ago

This is some really nice work here! πŸ‘

I haven't tested this, but everything looks great and is ready to merge I think. Just to clarify, this wouldn't break any current implementations?

Edit: This looks like a great foundation for a 2.0.0 release. I've created a Milestone of things I'd like in 2.0.0

mikkelu commented 5 years ago

Thanks! Yes, I also think it would be great for a 2.0.0 release πŸ‘

I am almost certain that it won't break any current implementations however, I am not 100% sure as our internal usage of this, is based on the changes in this PR.

mikkelu commented 5 years ago

We just hit another interesting obstacle. We need to do some custom sorting, but we cannot call that on req.paginate(on: req, transform{}), and we end up with conflicts between our custom sorts with the default sort.

I thought that maybe we should, for the pagination with the transformation closure, remove the sorting, because users can do their sorting in the transformation closure as they please.

This will allow for even more customization and still keep the default sorting for basic pagination (queries without the transformation closure handler).

What do you think?

anthonycastelli commented 5 years ago

@mikkelu I like that approach there. If you use the closure, everything is up to the user to customize then. That allows the extreme flexibility and control.

anthonycastelli commented 5 years ago

I have been chatting with @tanner0101 about this and we're going to work on merging this into Fluent itself, so within this pull request, we should think about how we can split this up between the core foundation and the fluff that the custom retransform closure will allow.

mikkelu commented 5 years ago

@anthonycastelli @tanner0101 that sounds like a great idea. I think it would be an awesome feature for Fluent. What are your initial thoughts on how to proceed?

anthonycastelli commented 5 years ago

Well, looking over your pull request, we'd want to split it up based on the core functionality and then any additional wrappers & extensions to implement the custom transformations. I have some time this weekend so I will sit down and start looking over it all and seeing how we can split it up. Just a quick overlook of it all, we shouldn't have an issue doing it.

Prince2k3 commented 5 years ago

@mikkelu I've been using your branch since it was posted, cause it was what I was looking for, and it has been cake to use. Amazing job!

anthonycastelli commented 5 years ago

Im gonna merge this into the master branch so it makes it easier for me to start working on the migration for Fluent 4

I'll also release a pre-release version of this from this master branch so everyone can start using it if they wish

mikkelu commented 5 years ago

Cool. Let me know if you need any help @anthonycastelli πŸ‘

anthonycastelli commented 5 years ago

Will do @mikkelu πŸ™‚