cakephp / elastic-search

Elastic search datasource for CakePHP
Other
88 stars 53 forks source link

PaginatorComponent #37

Closed damianoporta closed 4 years ago

damianoporta commented 9 years ago

I have adapted the paginator to handle elastic search queries. Very close to the original with only few changes regarding the counting.

markstory commented 9 years ago

I would think we should be generalizing the core paginator component, so it can work with other repository implementations. This implies that each custom data source will need to reimplement pagination which I don't think should be something we aim for.

Would it make sense to either adapt the query object in this oroject to provide the required features, or make paginatorcomponent less sql specific?

lorenzo commented 9 years ago

@markstory We could make the paginator work like the form helper. It would have detectors for selecting what methods to execute.

I'm very much interested in making the paginator aware of the elastic search features, such not having to call count() and the possibility of using a cursor id for fetching the next page

markstory commented 9 years ago

@lorenzo Would that be implemented as having a paginator in this repo though?

lorenzo commented 9 years ago

@markstory It depends... If we do the necesary changes in cakephp, then we only need to have an adapter in this repo. But with the current state of the code, having a paginator component extending the one in core (or completely replacing it) seems to be the best bet

markstory commented 9 years ago

Is using cursors for pagination the hard part? I don't see how the count() issue is not solvable with the current code. The query/resultset ckasses could have a method that doesn't do a separate request.

Is the cursor support also something we should generalize in the cakephp repo? Could other datastores also benefit from cursors?

lorenzo commented 9 years ago

@markstory postgres could also benefit from cursors (actually even mysql if you implement virtual cursors) I think it is an idea worth exploring

markstory commented 9 years ago

Ok, I would rather generalize the concept of paginating with cursors instead of duplicating a pagination component in this plugin. I feel that would let us avoid tough decisions around where class/feature boundaries should be and furthermore creates a world where each datasource needs its own way to do pagination which would be :cry:

damianoporta commented 8 years ago

news?

markstory commented 8 years ago

@damianoporta I'm still not keen on adding a paginationcomponent here. I would prefer we continue to generalize the interfaces so that the stock paginator can work with both elasticsearch and the ORM. I've not had much time to dedicate to that though.

damianoporta commented 8 years ago

@markstory should I use from/size directly for the moment?

markstory commented 8 years ago

@damianoporta Yeah, if you know of specific issues we can shore up, I'm happy to work on fixing them. However, I don't have much time to spend on uncovering all the issues.

Marlinc commented 8 years ago

Is this issue still relevant? If so, what needs to change in the core PaginatorComponent to make it possible to use it with this plugin?

lorenzo commented 8 years ago

It is relevant. I my opinion it is better to have the component in this plugin than having no way to paginate at all :P

Marlinc commented 8 years ago

I thought it might have been fixed by some unknown way ;) What's still missing from the core to be able to paginate over ElasticSearch stuff?

lorenzo commented 8 years ago

It needs to be abstracted in a way that it does not call as many table methods as it does now. For example calling alias() and prefixing the column with the alias is problematic.

In the specific case of ElasticSearch, it is also a problem that it issue a count() query, when by default elastic will return the total count in the same response

markstory commented 8 years ago

@lorenzo Shouldn't pagination be less coupled to relational databases though? Building a pagination component here leads me to believe that ever non-relational store would need its own PaginatorComponent, which makes me sad.

lorenzo commented 8 years ago

@markstory it should, but for sql it still needs to protect against field injection, correctly aliasing the columns, and issue 2 different queries to get the right results. None of that is needed for elastic

Marlinc commented 8 years ago

I'm not sure what's different between the ElasticSearch plugin and the Webservice plugin but the Webservice plugin can work with the PaginateComponent out of the box: image

Marlinc commented 8 years ago

I've looked into it a bit more, the count method in the Query currently gets the total amount of results from a custom ResultSet object that we have: https://github.com/UseMuffin/Webservice/blob/master/src/Query.php#L420-L431

We then store the total amount of results in the ResultSet when instantiating it. This works out of the box because of: https://github.com/cakephp/cakephp/blob/master/src/Controller/Component/PaginatorComponent.php#L173

Could something like that work for the ElasticSearch plugin?

markstory commented 8 years ago

@Marlinc I bet something like that would help.

qqqqb commented 8 years ago

Hello, I did my own pagination function suitable for any component. I hope you like it

    public function Pagination ($total = 0, $current = 1, $limit = 25, $numberOfPages = 9) {
    $url = [
        'controller' => $this->request->controller,
        'action' => $this->request->action,
    ];

    foreach ($this->request->pass as $pass)
        $url[] = $pass;

    $parms = $this->request->query;

    unset($parms['page']);

    foreach ($parms as $key => $value)
        $url['?'][$key] = $value;

    $pagesCount = ceil($total / $limit);
    $pagination = [
        'current'   => $current,
        'count'     => $total,
        'perPage'   => $limit,
        'prevPage'  => false,
        'nextPage'  => false,
        'pageCount' => $pagesCount,
        'pages'     => '',
        'prevHtml'  => '<li class="prev disabled"><a href="" onclick="return false;"> << ' . __('Previous').'</a></li>',
        'nextHtml'  => '<li class="next disabled"><a href="" onclick="return false;">' . __('Next').' >> </a></li>',
    ];

    if ($numberOfPages) {

        $pages = [];
        $halfnumberOfPages = intval($numberOfPages / 2) + 1;

        if ($current == 1) {
            $pages[] = 1;
            $numberOfPages = (($halfnumberOfPages - 1) * 2) + 1;

            if ($pagesCount < $numberOfPages)
                $numberOfPages = $pagesCount;

            for($i=2; $i < $numberOfPages + 1; $i++)
                $pages[] = $i;

        } else {
            for($i = 1; $i < $halfnumberOfPages; $i++) {
                $number = $current - $i;
                if ($number < 1)
                    break;
                if ($number >= $pagesCount)
                    $number = $pagesCount - $i + 1;
                $pages[] = $number;
            }

            $pages = array_reverse($pages);
            $pages[] = $current;

            $total = ($halfnumberOfPages-count($pages)) + $halfnumberOfPages;

            for($i = 1; $i < $total; $i++) {
                $number = $current + $i;
                if ($number > $pagesCount)
                    break;
                $pages[] = $number;
            }

            $missingPages = $numberOfPages - count($pages);
            for($i = $missingPages; $i > 0; $i--) {
                $number = $pages[0] - 1;
                if ($number < 1)
                    break;
                array_unshift($pages,$number);
            }
        }

        foreach($pages as $page) {
            $url['?']['page'] = $page;
            if ($page == $current)
                $pagination['pages'] .= '<li class="active"><a href="" onclick="return false;">'.$current.'</a></li>';
            else
                $pagination['pages'] .= '<li><a href="'.Router::url($url).'">' .$page.'</a></li>';
        }
    }

    if ($current > 1) {
        $pagination['prevPage'] = True;
        $url['?']['page'] = $current - 1;
        $pagination['prevHtml'] = '<li class="prev"><a rel="prev" href="'.Router::url($url).'"> << ' . __('Previous').'</a></li>';
    }

    if ($current < $pagesCount) {
        $pagination['nextPage'] = True;
        $url['?']['page'] = $current + 1;
        $pagination['nextHtml'] = '<li class="next"><a rel="next" href="'.Router::url($url).'">' . __('Next').' >> </a></li>';
    }

    return $pagination;
}
burzum commented 7 years ago

Now that the CakePHP interface for paginator abstraction is done, can we implement this?

I'm close in getting this to work https://gist.github.com/burzum/31bc2d672b75d7de52960f92de8f1325

markstory commented 7 years ago

@burzum That sounds like a great idea.

josbeir commented 6 years ago

@burzum @markstory is this still relevant?

burzum commented 6 years ago

@josbeir I would like to see it in the plugin, feel free to create a PR.

josbeir commented 6 years ago

will do, when i reach the point in my project when i'll be needing it that is :-)

Is the implementation you made a few comments up a good starting point (as in, does it work and is it a good approach ?) or should we be dealing with this from another perspective?

lorenzo commented 6 years ago

@josbeir we should be using the new Paginator class in the datasource package

josbeir commented 6 years ago

As discussed many times before, with the current implementation of cake's internal paginator it is almost impossible to make a quick fix for this:

Therefore, as a drop in replacement i made a modified version of the cake paginator that enables mapping sort fields (as sorting is the main problem currently):

https://github.com/josbeir/cakephp-paginator-sortmap

Instead of using 'whitelist' you can define a sortMap array that maps sortKeys with their corresponding keys inside the index. No aliasing is applied on those keys, and keys can also be combined to have multiple sort conditions under one key.

It works perfectly with ES (after defining a mapping), also works with cake's db ORM

burzum commented 6 years ago

As discussed many times before, with the current implementation of cake's internal paginator it is almost impossible to make a quick fix for this:

The internal behavior should be fixed then.

github-actions[bot] commented 4 years ago

This pull request is stale because it has been open 30 days with no activity. Remove the stale label or comment on this issue, or it will be closed in 15 days