[4.0][webservice] API pagination links #27005

stefanoel · 2019-11-06T12:15:52Z

Pull Request for Issue #27004 .

Summary of Changes

\libraries\src\MVC\View\JsonApiView.php was modified adding model->setState because 'start' and 'limit' weren't populated and they always got default values (0 and 20) showing the first 20 records.
I don't know if there's a better solution than this one to achieve the same result...
Furthermore, I've added checks to omit unnecessary pagination links.

com_config APIs now return correct links.

I'm not sure about the best way/place to encode square brackets...

Testing Instructions

\libraries\src\MVC\View\JsonApiView.php

/api/index.php/v1/plugins
/api/index.php/v1/plugins?page[offset]=10&page[limit]=10 (a generic page)
/api/index.php/v1/plugins?page[offset]=110&page[limit]=10 (last page)

\api\components\com_config\View\Application\JsonapiView.php

/api/index.php/v1/config/application

\api\components\com_config\View\Component\JsonapiView.php

/api/index.php/v1/config/com_content
/api/index.php/v1/config/com_content?page[offset]=40&page[limit]=20 (a generic page)
/api/index.php/v1/config/com_content?page[offset]=60&page[limit]=20 (last page)

Expected result

Actual result

Documentation Changes Required

API com_config now return correct links for pagination. Similarly, fixed JsonApiView.php. In JsonApiView I added model->setState because 'start' and 'limit' weren't populated and they always got default values (0 and 20) showing the first 20 records. I don't know if there's a better solution than this one...

…ms-wstest into ws-pagination

brianteeman · 2019-11-06T23:11:43Z

From what is described this PR does exactly what it says. I dont know enough to determine if it is correct to remove the unnecessary pagination links but it does what it says

wilsonge · 2019-11-06T23:24:34Z

libraries/src/MVC/View/JsonApiView.php

+		$currentPageQuery = $currentUrl->getVar('page', $currentPageDefaultInformation);
+
+		// Set model start and limit params
+		$model->setState('list.start', $currentPageQuery['offset']);


This should proxy through from the code here https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/MVC/Controller/ApiController.php#L214

I'm not sure I understood what you mean...
I moved $model->setState from JsonApiView.php to ApiController.php, using values obtained from the input page param (https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/MVC/Controller/ApiController.php#L201), and it seems to work fine.
So, can I commit these changes?
But in that case, I'd like to ask some clarifications on how the pagination params are stored in $this->input

joomla-cms/libraries/src/MVC/Controller/ApiController.php

Lines 202 to 214 in 8635691

$internalPaginationMapping = [];

if (\array_key_exists('offset', $paginationInfo))

{

$this->input->set('limitstart', $paginationInfo['offset']);

}

if (\array_key_exists('limit', $paginationInfo))

{

$internalPaginationMapping['limit'] = $paginationInfo['limit'];

}

$this->input->set('list', $internalPaginationMapping);

Here is used an array ($internalPaginationMapping) to store only the limit param, and moreover, dumping $this->input I noticed that the array isn't stored as associative, 'limit' key is dropped. Could these line be reviewed or this way of store pagination params in input are mandatory?

wilsonge · 2019-11-06T23:41:25Z

I dont know enough to determine if it is correct to remove the unnecessary pagination links but it does what it says

JSON API as a spec doesn't have strong opinions https://jsonapi.org/format/#fetching-pagination it just says "Keys MUST either be omitted or have a null value to indicate that a particular link is unavailable.". My feeling is first/last as a link is available so should be present (even if it's the current page). But I would definitely agree that having previous/next as nullable makes sense. As with all these subjective things - it's always down to personal preference :)

brianteeman · 2019-11-07T07:55:45Z

My feeling is first/last as a link is available so should be present (even if it's the current page)

In the gui they would be disabled

noccioletta · 2019-11-07T10:36:28Z

Hi, I'd like to better understand your responses:
is the workaround proposed by @stefanoel consistent or not ?
Is it worthwhile for Joomla4 ?

wilsonge · 2019-11-07T14:03:42Z

The changes to next/previous page is a good change. I think the first/last links should always be present.
I also think the view shouldn't be setting state into the model - it should be handled in the controller already.

chrisdavenport · 2019-11-07T15:10:38Z

The question of whether to include first/last/next/previous links on pages where they are redundant is one I've thought about before. As a rule of thumb I prefer to avoid them. My reasoning is that links are pointers to possible state transitions (that's what HATEOAS is all about), but redundant links are a bit like empty promises; it looks like they should lead to a state transition but actually they just lead straight back to the current state.

The client needs to know definitively that it has reached the first/last page when paginating. Including redundant links means that the client must compare the current URI with the first/last link URI and decide if they're the same or not. This can sometimes be tricky if the request URI can include arguments that don't affect the result set: how does the client know to ignore them when URI structure is supposed to be opaque? A simple string comparison may not suffice; it must reduce both to a canonical form before the comparison. It's only really safe to include redundant links if there are other signals that the client can use to decide whether it needs to follow a link or not. For example, if there are separate pageNumber and pageCount attributes returned in the response.

So, in my opinion, if you include redundant links, you should also include pageNumber and pageCount (or something similar) and encourage developers not to do URI comparisons. On the other hand, omitting redundant links doesn't mean that you should omit pageNumber/pageCount as they can be used by clients that want to randomly access pages.

So my preference is to omit redundant links but include pageNumber/pageCount (or better still something like Offset and totalCount as the concept of a page can be a bit problematic). And on a first pass at least, assume that the data is essentially static so you don't have to worry about insertions and deletions accidentally resulting in duplications and omissions when paginating; that's a whole other problem to solve.

wilsonge · 2019-11-07T15:20:45Z

There already is a total-pages property available.

The client needs to know definitively that it has reached the first/last page when paginating

That's why you use the next/previous attributes for in JSON API

This can sometimes be tricky if the request URI can include arguments that don't affect the result set

These are always going to be query parameters. If the URL path is physically different you cannot guarantee the data is the same (and in our core Joomla response this is the case anyhow for the purposes of said checks - which is the more concrete thing - because ultimately people are parsing our responses).

I don't see first and last pages are not necessarily redundant even if they are the same page. Previous/Next definitely would be redundant because it's not actually a next/previous page if they don't exist

Removed $model->setState from the view, everything seems to work... ToDo: square brackets encoding?

I introduced the method "queryEncode" to encode square brackets in pagination links. AbstractURI->toString() returns unencoded brackets, so I had to find a workaround, but I'm not sure it's the cleanest way to do this. The method is defined as protected in Joomla\CMS\MVC\View\JsonApiView so that it can be used by other extending classes, like com_config api views. Fixed an error in com_config component view pagination (L109).

stefanoel · 2019-11-13T15:27:04Z

About square brackets encoding

Pagination links have unencoded square brackets, due to AbstractURI buildQuery method (https://github.com/joomla-framework/uri/blob/eed89d4997fcc52c08bba56116fa3ac4aef6a9db/src/AbstractUri.php#L345-L348) as it was already reported here

Bug in AbstractUri::buildQuery - invalid HTML emitted ('[' and ']' not encoded in tagged elements list) #17365
Bug in AbstractUri::buildQuery - invalid HTML emitted ('[' and ']' not encoded in tagged elements list) joomla-framework/uri#21
Tagged elements list emits unencoded '[' and ']' (invalid HTML) #17966

so, to comply with JSON API spec, I chose to handle the encoding with an ad hoc protected method (I've also updated the com_config APIs to use it)
https://github.com/stefanoel/joomla-cms-wstest/blob/5df64298d7445ba3183923b00e65bcf5df434e1e/libraries/src/MVC/View/JsonApiView.php#L265

Now the links returned have encoded square brackets.
Any suggestions?

Ankit7Das · 2020-01-20T14:58:33Z

I wish to work on it

wilsonge · 2020-03-20T17:30:27Z

@stefanoel apologies this one dropped off my radar. are you willing to fix the conflicts here so i can do a round of testing and get it in please

api/components/com_config/View/Application/JsonapiView.php

libraries/src/MVC/View/JsonApiView.php

Co-Authored-By: Quy <quy@fluxbb.org>

stefanoel · 2020-03-20T18:30:38Z

sorry, now I'm stuck and I can't undestand the problem... I can't access my workstation and I'm not used to solve things from web interface... can you help please?

Co-Authored-By: Quy <quy@fluxbb.org>

Razzo1987 · 2020-03-23T18:46:49Z

I have not tested this item.

When I try to apply the patch (Joomla! Patch Tester 3.0.0 beta 4):

Error
The file marked for modification does not exist: api/components/com_config/View/Application/JsonapiView.php

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27005.}

alikon · 2020-03-23T18:52:22Z

can i suggest you to use git commands to apply this patch
curl -L https://github.com/joomla/joomla-cms/pull/27005.diff | git apply

patchtester > Patch Tester 3.0.0 Beta 3 is still a WIP

anyway this pr needs conflict to be solved

alikon · 2020-03-23T18:53:14Z

@stefanoel can you fix conflict please

stefanoel · 2020-03-24T08:21:07Z

can i suggest you to use git commands to apply this patch
curl -L https://github.com/joomla/joomla-cms/pull/27005.diff | git apply

patchtester > Patch Tester 3.0.0 Beta 3 is still a WIP

anyway this pr needs conflict to be solved

Hi @alikon can you suggest how to do it from web interface? I'm working from home due to coronavirus emergency here in Italy, and I have to use a smartphone these days. I can't access my workstation...

richard67 · 2020-03-24T10:19:01Z

@wilsonge Could you ckeck the conflict in the ApiController? Can it be that the pagination link is already ok with ~~recently~~ merged PR #27696 ? Or does it still need the change from this PR here in addition? I've stumpled over this when checking the merge conflicts here to 4.0-dev.

…l-ws-pagination-1

richard67 · 2020-03-24T10:49:43Z

@stefanoel I've made a pull request in your repository: stefanoel#1. This will update your PR here so it is up to date with its base branch, and solves the merge conflicts shown here on GitHub.

@wilsonge Please review the changes in libraries/src/MVC/Controller/ApiController.php as I mentioned above.

Merge upstream/4.0-dev into ws-pagination

richard67 · 2020-03-24T11:00:00Z

@wilsonge @alikon Conflicts solved. Ready for review and testing.

alikon · 2020-03-24T11:09:29Z

great team-working
f***** covid19

richard67 · 2020-03-24T11:12:24Z

@wilsonge Drone failing the 2nd time now. No idea if related.

wilsonge · 2020-04-04T11:52:41Z

This looks good to me! Thankyou!

joomla-cms-bot added the PR-4.0-dev label Nov 6, 2019

stefanoel added 3 commits November 6, 2019 13:29

Merge branch '4.0-dev' into ws-pagination

c03eb20

Code standard fixes

0f8ea37

Merge branch 'ws-pagination' of https://github.com/stefanoel/joomla-c…

74e79b9

…ms-wstest into ws-pagination

Quy mentioned this pull request Nov 6, 2019

[RFC][4.0] API pagination links #27004

Closed

wilsonge reviewed Nov 6, 2019

View reviewed changes

stefanoel added 5 commits November 8, 2019 12:11

Merge branch '4.0-dev' into ws-pagination

bb33997

Updated ApiController.php displayList() pagination

90c6acf

Update JsonApiView.php

ca331aa

Removed $model->setState from the view, everything seems to work... ToDo: square brackets encoding?

Merge branch '4.0-dev' into ws-pagination

5df6429

stefanoel added 2 commits November 25, 2019 15:49

Merge branch '4.0-dev' into ws-pagination

727d641

Merge branch '4.0-dev' into ws-pagination

00c1b1d

alikon added the GSoC label Jan 15, 2020

alikon removed the GSoC label Feb 23, 2020

Update JsonapiView.php

f5f01b1

Quy reviewed Mar 20, 2020

View reviewed changes

api/components/com_config/View/Application/JsonapiView.php Outdated Show resolved Hide resolved

Quy reviewed Mar 20, 2020

View reviewed changes

libraries/src/MVC/View/JsonApiView.php Outdated Show resolved Hide resolved

Update libraries/src/MVC/View/JsonApiView.php

32d57cf

Co-Authored-By: Quy <quy@fluxbb.org>

stefanoel closed this Mar 20, 2020

stefanoel reopened this Mar 20, 2020

Update api/components/com_config/View/Application/JsonapiView.php

9b096b3

Co-Authored-By: Quy <quy@fluxbb.org>

alikon added the Conflicting Files label Mar 23, 2020

richard67 added 2 commits March 24, 2020 11:22

Merge remote-tracking branch 'upstream/4.0-dev' into 4.0-dev-stefanoe…

ecd980e

…l-ws-pagination-1

Add back removed line from conflicts solving

bc1b63b

richard67 mentioned this pull request Mar 24, 2020

Merge upstream/4.0-dev into ws-pagination stefanoel/joomla-cms-wstest#1

Merged

Merge pull request #1 from richard67/4.0-dev-stefanoel-ws-pagination-1

8d5d1e0

Merge upstream/4.0-dev into ws-pagination

wilsonge added 2 commits March 24, 2020 11:30

Fix up the code to latest version

6dec7cc

More fixes

c42d641

alikon removed the Conflicting Files label Mar 26, 2020

alikon mentioned this pull request Mar 26, 2020

Add ready to install packages to each PR #28409

Merged

wilsonge merged commit 19578aa into joomla:4.0-dev Apr 4, 2020

wilsonge added this to the Joomla 4.0 milestone Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.0][webservice] API pagination links #27005

[4.0][webservice] API pagination links #27005

stefanoel commented Nov 6, 2019

brianteeman commented Nov 6, 2019

wilsonge Nov 6, 2019

stefanoel Nov 7, 2019

wilsonge commented Nov 6, 2019 •

edited

Loading

brianteeman commented Nov 7, 2019

noccioletta commented Nov 7, 2019

wilsonge commented Nov 7, 2019

chrisdavenport commented Nov 7, 2019

wilsonge commented Nov 7, 2019

stefanoel commented Nov 13, 2019

Ankit7Das commented Jan 20, 2020

wilsonge commented Mar 20, 2020

stefanoel commented Mar 20, 2020

Razzo1987 commented Mar 23, 2020

alikon commented Mar 23, 2020

alikon commented Mar 23, 2020

stefanoel commented Mar 24, 2020

richard67 commented Mar 24, 2020 •

edited

Loading

richard67 commented Mar 24, 2020

richard67 commented Mar 24, 2020

alikon commented Mar 24, 2020

richard67 commented Mar 24, 2020

wilsonge commented Apr 4, 2020

	$internalPaginationMapping = [];

	if (\array_key_exists('offset', $paginationInfo))
	{
	$this->input->set('limitstart', $paginationInfo['offset']);
	}

	if (\array_key_exists('limit', $paginationInfo))
	{
	$internalPaginationMapping['limit'] = $paginationInfo['limit'];
	}

	$this->input->set('list', $internalPaginationMapping);

[4.0][webservice] API pagination links #27005

[4.0][webservice] API pagination links #27005

Conversation

stefanoel commented Nov 6, 2019

Summary of Changes

Testing Instructions

\libraries\src\MVC\View\JsonApiView.php

\api\components\com_config\View\Application\JsonapiView.php

\api\components\com_config\View\Component\JsonapiView.php

Expected result

Actual result

Documentation Changes Required

brianteeman commented Nov 6, 2019

wilsonge Nov 6, 2019

Choose a reason for hiding this comment

stefanoel Nov 7, 2019

Choose a reason for hiding this comment

wilsonge commented Nov 6, 2019 • edited Loading

brianteeman commented Nov 7, 2019

noccioletta commented Nov 7, 2019

wilsonge commented Nov 7, 2019

chrisdavenport commented Nov 7, 2019

wilsonge commented Nov 7, 2019

stefanoel commented Nov 13, 2019

Ankit7Das commented Jan 20, 2020

wilsonge commented Mar 20, 2020

stefanoel commented Mar 20, 2020

Razzo1987 commented Mar 23, 2020

alikon commented Mar 23, 2020

alikon commented Mar 23, 2020

stefanoel commented Mar 24, 2020

richard67 commented Mar 24, 2020 • edited Loading

richard67 commented Mar 24, 2020

richard67 commented Mar 24, 2020

alikon commented Mar 24, 2020

richard67 commented Mar 24, 2020

wilsonge commented Apr 4, 2020

wilsonge commented Nov 6, 2019 •

edited

Loading

richard67 commented Mar 24, 2020 •

edited

Loading