vapor-community / pagination

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

Fluent filters are missing #2

Closed gperdomor closed 6 years ago

gperdomor commented 6 years ago

With the current API I can't do something like User.query(on: req).filter(\.email == searchTerm).paginate(for: req) in a searchendpoint because paginate(for: req) call the all() method.

My proposal is that Pagination package handle only the split and page build without querying stuff, more like an Array extension maybe 🤔 , in that way you can access to the full fluent API and do something like this:

func index(_ req: Request) throws -> Future<Paginated<User>> {
    return User.query(on: req).all().paginate(for: req)
}

func search(_ req: Request) throws -> Future<Paginated<User>> {
    guard let searchTerm = req.query[String.self, at: "email"] else {
        throw Abort(.badRequest)
    }

    return try User.query(on: req).filter(\.email == searchTerm).paginate(for: req)
}
anthonycastelli commented 6 years ago

When you do

func index(_ req: Request) throws -> Future<Paginated<User>> {
    return User.query(on: req).paginate(for: req)
}

The results should be returned paginated. You don't need to append the .all() before paginating.

gperdomor commented 6 years ago

Yeah, that's fine... the problem is when I need filter things... Like this...

return try User.query(on: req).filter(\.name == searchTerm).paginate(for: req)

I need some users with some names, I can't paginate that results

anthonycastelli commented 6 years ago

I'll have to look into this again, but last I had checked, I was just fetching the request from the query already built. https://github.com/vapor-community/pagination/blob/master/Sources/Pagination/QueryBuilder%2BPaginatable.swift#L13. Does it just fetch all of the results regardless of the search term?

anthonycastelli commented 6 years ago

Ahh, I see whats going on here. Let me work out a fix.

gperdomor commented 6 years ago

In my case always get all the users...

My code:

func search(_ req: Request) throws -> Future<Paginated<User>> {
        guard let searchTerm = req.query[String.self, at: "email"] else {
            throw Abort(.badRequest)
        }

        return try User.query(on: req).filter(\.email == searchTerm).paginate(for: req)
}

The results

screen shot 2018-04-30 at 5 43 16 pm
gperdomor commented 6 years ago

I think the problem is the line 40 when you call all()

anthonycastelli commented 6 years ago

The issue is from here https://github.com/vapor-community/pagination/blob/677b712da51d230a1b3c5a5971a470803274dc43/Sources/Pagination/Model%2BPaginatable.swift#L16-L20. Whats happening is the Model (Self) is reconstructing its own query. Let me work out a fix. 🙂

gperdomor commented 6 years ago

I can't convert my users to a PublicUser struct (to exclude password field in the response) without conform Model in the PublicUser, maybe I'm not doing in the correct way... Can you validate that please?

anthonycastelli commented 6 years ago

Regarding your issue for converting to a struct, currently, there is no "smart" way to handle that. Once Fluent is tagged and Tanner has finned releasing Vapor 3, I'm gonna bug him with some ideas on how to approach this one.

gperdomor commented 6 years ago

@anthonycastelli any progress with your last comment?