Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(graphql): implement real cursor pagination #9387

Merged
merged 14 commits into from
Oct 18, 2023

Conversation

tchak
Copy link
Member

@tchak tchak commented Aug 3, 2023

Aujourd'hui, la pagination sur l'API est implémentée avec une limite et un décalage. Cela pose des problèmes lors de la synchronisation des dossiers. Par exemple : on récupère les deux premières pages, un dossier est supprimé sur la première page, maintenant la troisième page est décalée d'un élément. La solution consiste à encoder dans le curseur le dernier élément récupéré au lieu d'un décalage. Cela rend la pagination "stable". Pas de changement d'API car ce que nous exposons est déjà une pagination par curseur. Nous changeons simplement le contenu du curseur.

L'implémentation du calcul des paramètres de page est essentiellement copié d'ici : https://github.com/hayes/pothos/blob/26b4dd83777273d4aa0af6ac97e750c1363050c1/packages/plugin-relay/src/utils/connections.ts#L194-L220

@what-the-diff
Copy link

what-the-diff bot commented Aug 3, 2023

PR Summary

  • Addition of new variables and deprecated old ones in Stored Queries
    The way our system interacts with the GraphQL queries has been updated. Several new variables were added and old ones have been marked as discontinued.

  • Created class for Cursor-Based pagination
    In a bid to enhance the efficiency of how we manage pages and load nodes in a connection, a new class was added specifically for cursor-based pagination.

  • Established a management class for Deleted Files
    There is now a dedicated separate class that manages deleted dossiers (files) while also utilizing cursor-based pagination.

  • Modified DossiersConnection class
    Updates were made to our DossiersConnection class to improve how we handle sorting and the overall managing of dossier pages.

  • Added class for Pending Deleted Files
    In order to better handle files waiting to be deleted, a new class was created. This class also benefits from cursor-based pagination to efficiently handle these files.

  • Changes to the schema
    Further improvements include the adding of a new directive to enforce that exactly one field must be supplied and not be null. Also, argument changes in several fields are now deprecated with new description and a suggestion to use the last argument instead.

  • Updates in Dossier Types
    The way the system handles dossiers, as well as deleted and pending deleted dossiers, has been enhanced through the use of new classes.

  • Cosmetic changes to GroupeInstructeurWithDossiers Type
    The changes here are quite similar to the dossier type updates, focusing on improved management of dossiers.

  • Updates to Dossier Models
    This includes changes that were made to how ordering is handled in Dossier Models. It also includes a replacement of previous methods of ordering dossiers.

@tchak tchak force-pushed the graphql-pagination-fix branch from 51ad286 to 6ef8720 Compare August 3, 2023 08:31
Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: la transition va casser les requetes en cours qui vont passer de l'ancien cursor au nouveau.


def nodes
load_nodes
@nodes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@nodes

app/graphql/connections/cursor_connection.rb Outdated Show resolved Hide resolved
app/graphql/connections/cursor_connection.rb Outdated Show resolved Hide resolved
@tchak
Copy link
Member Author

tchak commented Aug 8, 2023

Note: la transition va casser les requetes en cours qui vont passer de l'ancien cursor au nouveau.

Ça ne serait pas un énorme effort d'implémenter le fallback. Tu penses que ça vaut le coup ?

@tchak tchak force-pushed the graphql-pagination-fix branch from 6ef8720 to f5997d0 Compare August 8, 2023 09:01
@tchak tchak force-pushed the graphql-pagination-fix branch 6 times, most recently from 43d3568 to 8cdb3f0 Compare August 29, 2023 13:54
@tchak tchak force-pushed the graphql-pagination-fix branch from 8cdb3f0 to 93a5bc3 Compare October 13, 2023 11:24
@tchak
Copy link
Member Author

tchak commented Oct 13, 2023

@LeSim, c'est bon pour une dernière revue et merge pour moi. J'ai simplifié le code dans compute_page_info - je trouve que ça se lit mieux maintenant. J'ai aussi fait du dessin avec @mfo hier. Ça m'a permis de confirmer que les cas de tests sont bons. J'ai également comparé les cas de tests avec l'implémentation existante et je confirme que c'est bon.

@tchak tchak force-pushed the graphql-pagination-fix branch from 8ef368b to a43176b Compare October 18, 2023 11:03
@tchak tchak enabled auto-merge October 18, 2023 11:03
@tchak tchak added this pull request to the merge queue Oct 18, 2023
Merged via the queue into demarches-simplifiees:main with commit 6ff2287 Oct 18, 2023
15 checks passed
@tchak tchak deleted the graphql-pagination-fix branch October 18, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants