[VOTE] Explicit Paginator return type

[ronan-gloo]

Summary

We are used to use abstractions as return type in our method declaration. In most of the cases it is a good practice because we can change implementation without breaking the client.

Usually, presentation repository interfaces declare fetchAll like methods, which allow to retrieve a collection of models based on filters. This is a typical declaration we can find in API project :

<?php
namespace ShoppingFeed\Data\Presentation;

interface DataAccessInterface
{
    public function fetchAllBy(int $userId): iterable
}

The assumption and expectations for someone who call $access->fetchAllBy(123) is to get an iterable set for all elements that match the constrain, here $userId.

As the return type provides an iterable type, the only thing the client can do is to... iterates on:

<?php
$set = $access->fetchAllBy(123);

// Only possible thing we can do with iterables
foreach ($set as $item) {
    echo $item->get('id') . "\n"
}

Looking the code above, we all expect to loop accross an array or \Iterator which provides what we asked for: all data for $userId.

But this is not what we do : Our DBAL (or whatever) implementations usually provide a ShoppingFeed\Paginator\PaginatorAdapterInterface or ShoppingFeed\Paginator\PaginatorInterface, which limit the number of items to fetch.

So, if we process the same client code with a paginator :

<?php
$set = $access->fetchAllBy(123);
foreach ($set as $item) {
    echo $item->get('id') . "\n"
}

We only got the first 10 results of the set by default.

• This is clearly an unexpected behavior that can lead to buggy / broken implementation.
• This is a code smell as infrastructure implémentations does not respect what the interface suggest

Valid Code

As we still want to break result in chunk in most of the cases (paginated API's), we need to change the interface signatures and make the contract clear between client and implementation.

interface DataAccessInterface
{
    public function fetchAllBy(int $userId): \ShoppingFeed\Paginator\PaginatorInterface
}

Now the clients nows it will be limited results when iterating on, and may controls it if desired.

PaginatorAdapterInterfaces have to stay to implementation level and not be exposed to interfaces, as we have no guaranties about their default behavior when they are not wrapper into a Paginator instance.

Invalid Code

Do not type againts iterable, \Traversable,\Iterator or \IteratorAggregate types when expecting a limited set.
Example:

<?php
namespace ShoppingFeed\Data\Presentation;

interface DataAccessInterface
{
    public function fetchAllBy(int $userId): iterable
}

Exemples of code to fix

• Taxonomy Interface : https://github.com/shoppingflux/api/blob/5137ec4a1bf236fc6736409117075b47c61473a9/project/Channel/src/Application/Read/TaxonomyAccessInterface.php#L13
• Taxonomy implementation : https://github.com/shoppingflux/api/blob/5137ec4a1bf236fc6736409117075b47c61473a9/project/Channel/src/Infrastructure/Dbal/ReadTaxonomyAccess.php#L37-L43

Documentation

• Paginator library : https://github.com/shoppingflux/paginator/blob/master/docs/index.md
• Iterable type : https://www.php.net/manual/en/language.types.iterable.php

[william-leny]

+1

[ddattee]

L'utiliter du PaginatorAdapter est de rendre transparent la pagination pour l'iteration non ?

Du coup pour moi le soucis c'est pas tant le type retour que l'adapter qui devrait parcourir toute les pages.
Et quand une limite est passé en "dur" lors de la requete (limit 10) et bien c'est normal qu'on ai que les 10 premier résultats.

[ronan-gloo]

L'utiliter du PaginatorAdapter est de rendre transparent la pagination pour l'iteration non ?

Non, son utilité est de paginer les résultats, donc d'appliquer une limite sur la requête de départ en fonction du backend (DBAL, Mongo, Elastic, etc). Il n'est pas franchement prévu de l'utiliser seul en fait, c'est simplement un adapter.

C'est plutôt le rôle du PaginatedIterator.

Du coup pour moi le soucis c'est pas tant le type retour que l'adapter qui devrait parcourir toute les pages.

Et donc avoir des millions de résultat à afficher pour certaines APIs.

Je renvoie sur la doc histoire que l'on sache bien tous comment ça fonctionne : https://github.com/shoppingflux/paginator/blob/master/docs/index.md

[ronan-gloo]

Je rebondi sur la remarque de David en me disant que pour que ça soit clair, il faudrait retourner un PaginatorInterface et non un PaginatorAdapterInterface

[ddattee]

Ok compris, j'étais pas dans le bon contexte: on veut garder la pagination dans ce cas et non juste itérer.
Dans ce cas oui je suis d'accord pour la mise a jour de signature.

Et je confirme que PaginatorInterface fait plus sens que l'adapter.

[mdumoulin]

Je me demande s'il ne faut pas plutôt voir la pagination comme un stratégie de l'implémentation plutôt que comme une contrainte donnée par l'interface.
On pourrait imaginer une autre implémentation de l'interface qui renvoie tous les résultats en parcourant le paginator pour un usage cli par exemple.

[ronan-gloo]

On a déjà tous les outils pour le faire en typant sur un paginateur, je ne suis pas certain qu'on ai a y gagner à tout prévoir côté repository et complexifier les implementations (car faudra donner un exemple de comment on pourrait faire) :

<?php
# cli.php file

$iterator = new PaginatedIterator($access->fetchAllBy(123));
$iterator->setItemsPerPage(500);

// Iterates over all elements for user 123
foreach ($iterator as $item) { echo $item->id; }

On le fait ici d'ailleurs dans l'API, même si ça devrait plutôt être initialisé dans le Stream : https://github.com/shoppingflux/api/pull/961/files#diff-690a1a7f5092fe35480aa35f2fed0c3fedd60d2eb3f0b5cf5bc4627c9548489bR34-R35

[mdumoulin]

On a déjà tous les outils pour le faire en typant sur un paginateur, je ne suis pas certain qu'on ai a y gagner à tout prévoir côté repository et complexifier les implementations

Ok, je comprends. Est-ce que dans ce cas, ça ne vaudrait pas le coup d'appeler explicitement ces méthodes 'paginate' plutôt que 'fetch' ? L'idée étant que 'fetch' est réservé pour ce qui retourne des iterable et 'paginate' pour ce qui renvoie un paginator. En tant qu'utilisateur, on sait directement quel type de résultat on va devoir manipuler.

[ronan-gloo]

Y a pas d'intérêt dans la mesure ou un new PaginatedIterator($this->repository->fetchAll()) vas te fournir un iterateur complet. Le type de retour suffit à connaitre la nature du résultat.

L'idée n'est pas de forcer des paginator partout, mais de les typer systématiquement quand ils sont utilisés, car c'est pas un détail d'implémentation de fournir un itérateur sur 10 résultats au lieu de la totalité de la table dans un fetchAll.