Ajouter un uuid au model Page (#67710) #25

Merged
lguerin merged 7 commits from wip/67710-page-uuid into main 2023-02-01 18:18:39 +01:00
Owner
No description provided.
lguerin force-pushed wip/67710-page-uuid from bb0f53a249 to a3b914858f 2023-01-06 16:33:24 +01:00 Compare
lguerin force-pushed wip/67710-page-uuid from a3b914858f to e90d3653ef 2023-01-09 17:04:04 +01:00 Compare
lguerin force-pushed wip/67710-page-uuid from e90d3653ef to bcd7b4c523 2023-01-09 17:09:51 +01:00 Compare
lguerin force-pushed wip/67710-page-uuid from bcd7b4c523 to 5fc10ccf83 2023-01-09 17:11:39 +01:00 Compare
lguerin changed title from WIP: Ajouter un uuid au model Page (#67710) to Ajouter un uuid au model Page (#67710) 2023-01-09 17:11:57 +01:00
lguerin reviewed 2023-01-09 17:14:08 +01:00
@ -0,0 +4,4 @@
from django.db import migrations
def natural_key(page):
Author
Owner

reprise de différentes méthodes du model Page, pour générer un uuid à partir de la chaine de slug

reprise de différentes méthodes du model Page, pour générer un uuid à partir de la chaine de slug
lguerin reviewed 2023-01-09 17:14:48 +01:00
@ -0,0 +38,4 @@
page.uuid = uuid.UUID(slug_hash.hexdigest()[:32])
if page.uuid in known_uuids:
# uuid unicity !
page.uuid = uuid.uuid4()
Author
Owner

au cas où, pour éviter les collisions et permettre à la migration suivante de passer (migration qui pose un unique=True sur uuid)

au cas où, pour éviter les collisions et permettre à la migration suivante de passer (migration qui pose un unique=True sur uuid)
Owner

Précaution digne du code d'un robot spatial :)

Précaution digne du code d'un robot spatial :)
lguerin reviewed 2023-01-09 17:15:47 +01:00
@ -0,0 +1,36 @@
from django.db import migrations
def forward(apps, schema_editor):
Author
Owner

Pour nettoyer le code: au lieu de faire la migration au load d'un snapshot, pour les anciennes cellules card, migration.
On pourra vider la migration et virer les tests (transactionnels, c'est long) plus tard.

Pour nettoyer le code: au lieu de faire la migration au load d'un snapshot, pour les anciennes cellules card, migration. On pourra vider la migration et virer les tests (transactionnels, c'est long) plus tard.
lguerin reviewed 2023-01-09 17:16:48 +01:00
@ -631,4 +630,0 @@
cell_data['fields']['display_mode'] = 'table'
cell_data['fields']['title_type'] = 'auto'
if cell_data['fields'].get('custom_title'):
cell_data['fields']['title_type'] = 'manual'
Author
Owner

ici, le code déplacé dans une migration

ici, le code déplacé dans une migration
lguerin reviewed 2023-01-09 17:19:15 +01:00
@ -0,0 +25,4 @@
for snapshot in PageSnapshot.objects.all():
changed = False
if (snapshot.serialization.get('fields') or {}).get('parent'):
Author
Owner

non nécessaire si on fait une restoration de snapshot, mais si on veut juste exporter un snapshot pour le visualiser et peut-être l'importer ailleurs, on aura le bon parent dans l'export.

non nécessaire si on fait une restoration de snapshot, mais si on veut juste exporter un snapshot pour le visualiser et peut-être l'importer ailleurs, on aura le bon parent dans l'export.
Owner

Il me semble qu'il manque un bout important pour que les anciens fichiers d'export continuent à pouvoir être importés (actuellement je pense que ça crash mais outre ne pas crasher ça serait bien de voir qu'il n'y a pas d'uuid, puis d'en ajouter un dynamiquement en faisant le calcul du hash comme dans la migration de 0001).

Il me semble qu'il manque un bout important pour que les anciens fichiers d'export continuent à pouvoir être importés (actuellement je pense que ça crash mais outre ne pas crasher ça serait bien de voir qu'il n'y a pas d'uuid, puis d'en ajouter un dynamiquement en faisant le calcul du hash comme dans la migration de 0001).
vdeniaud reviewed 2023-01-10 14:09:36 +01:00
@ -0,0 +28,4 @@
Page = apps.get_model('data', 'Page')
known_uuids = set(Page.objects.filter(uuid__isnull=False).values_list('uuid', flat=True))
for page in Page.objects.filter(uuid__isnull=True):
if page.snapshot is not None:
Owner

Pour ma culture, dans quel cas page.snapshot est vide ?

Pour ma culture, dans quel cas page.snapshot est vide ?
Author
Owner

Pour une page normale, page.snapshot est None.

Lorsqu'on fait "view" ou "export" sur un snapshot, ça génère une Page avec le snapshot attaché (qu'on ne sort dans le code classique, hors migration qui n'utilise pas les managers, que via le manager Page.snapshots)

Pour une page normale, page.snapshot est None. Lorsqu'on fait "view" ou "export" sur un snapshot, ça génère une Page avec le snapshot attaché (qu'on ne sort dans le code classique, hors migration qui n'utilise pas les managers, que via le manager Page.snapshots)
Owner

Pendant la grosse semaine où un changement est déployé en recette mais pas en production c'est techniquement assez régulier que certains exports/imports puissent échouer et parfois ça arrive, il y a alors un ticket et par exemple on modifie le fichier d'export pour qu'il puisse s'importer correctement et c'est bon ainsi.

Le cas que tu exposes est l'inverse mais je pense qu'on doit le traiter pareil, si jamais quelqu'un a un vieux fichier et l'essaie, ça échouera il y aura ticket on gérera ponctuellement. (plutôt qu'ajouter au code)

Pendant la grosse semaine où un changement est déployé en recette mais pas en production c'est techniquement assez régulier que certains exports/imports puissent échouer et parfois ça arrive, il y a alors un ticket et par exemple on modifie le fichier d'export pour qu'il puisse s'importer correctement et c'est bon ainsi. Le cas que tu exposes est l'inverse mais je pense qu'on doit le traiter pareil, si jamais quelqu'un a un vieux fichier et l'essaie, ça échouera il y aura ticket on gérera ponctuellement. (plutôt qu'ajouter au code)
Author
Owner

(on pourrait gérer assez facilement le cas où page.uuid est manquant, mais alors il faudrait aussi gérer page.parent, et les occurrences dans les cellules; ça devient vite une usine à gaz - du meme type que la migration 0064)

(on pourrait gérer assez facilement le cas où page.uuid est manquant, mais alors il faudrait aussi gérer page.parent, et les occurrences dans les cellules; ça devient vite une usine à gaz - du meme type que la migration 0064)
vdeniaud reviewed 2023-01-10 14:21:09 +01:00
@ -0,0 +9,4 @@
except (ValueError, AttributeError):
pass
else:
# it's a uuid, don't change it
Owner

Qu'est-ce qui justifie ce bout ? 0063_old_card_cells juste avant ?

En tout cas si une page a le malheur d'avoir un slug qui ressemble à un uuid ça va mal marcher.

Qu'est-ce qui justifie ce bout ? 0063_old_card_cells juste avant ? En tout cas si une page a le malheur d'avoir un slug qui ressemble à un uuid ça va mal marcher.
Author
Owner

Juste au cas où on ait passé la migration 0062 et redémarré le service avant de passer la 0064, pour une raison x ou y. Dans ce cas on commencera à trouver des uuid dans les snapshots, c'était pour que ça fonctionne.
(de même dans la 0061 je ne traite que les uuids manquants, en gardant un set des uuids trouvés et générés)
Tu penses qu'on pourrait avoir des slugs qui sont des uuids ? :)

Juste au cas où on ait passé la migration 0062 et redémarré le service avant de passer la 0064, pour une raison x ou y. Dans ce cas on commencera à trouver des uuid dans les snapshots, c'était pour que ça fonctionne. (de même dans la 0061 je ne traite que les uuids manquants, en gardant un set des uuids trouvés et générés) Tu penses qu'on pourrait avoir des slugs qui sont des uuids ? :)
Owner

Tu penses qu'on pourrait avoir des slugs qui sont des uuids ? :)

C'est peu probable j'en conviens mais ça paraît facile de s'en prémunir, par exemple en inversant les deux blocs pour que ce soit 1/ regarder le slug est dans page_uuids_by_slugs 2/ sinon, si c'est un uuid, le renvoyer, ou alors la version solide vraiment regarder si l'uuid est dans {page.uuid for page in pages} avant de le renvoyer.

Mais comme tu le sens :)

> Tu penses qu'on pourrait avoir des slugs qui sont des uuids ? :) C'est peu probable j'en conviens mais ça paraît facile de s'en prémunir, par exemple en inversant les deux blocs pour que ce soit 1/ regarder le slug est dans page_uuids_by_slugs 2/ sinon, si c'est un uuid, le renvoyer, ou alors la version solide vraiment regarder si l'uuid est dans {page.uuid for page in pages} avant de le renvoyer. Mais comme tu le sens :)
lguerin marked this conversation as resolved
vdeniaud reviewed 2023-01-10 14:22:49 +01:00
@ -0,0 +21,4 @@
def forward(apps, schema_editor):
PageSnapshot = apps.get_model('data', 'PageSnapshot')
Page = apps.get_model('data', 'Page')
page_uuids_by_slugs = {page.slug: str(page.uuid) for page in Page.objects.only('uuid', 'slug')}
Owner

Le slug d'une page n'étant pas unique, qu'est-ce qui donne confiance que ça marche ?

Le slug d'une page n'étant pas unique, qu'est-ce qui donne confiance que ça marche ?
Author
Owner

Je me suis fait la même remarque, mais dans le code précédent, pour tout ce qui est parent, et référence dans une cellule, on se contente de prendre le dernier bout de la chaîne et de chercher la page correspondante. Si plusieurs slugs, ça pète.
Là ça marchera, mais ça ne prendra peut-être pas la bonne page.

Je me suis fait la même remarque, mais dans le code précédent, pour tout ce qui est parent, et référence dans une cellule, on se contente de prendre le dernier bout de la chaîne et de chercher la page correspondante. Si plusieurs slugs, ça pète. Là ça marchera, mais ça ne prendra peut-être pas la bonne page.
Owner

OK si le cas slug dupliqué provoque un crash on peut raisonnablement supposer que ce n'est pas la peine de s'embêter avec.

OK si le cas slug dupliqué provoque un crash on peut raisonnablement supposer que ce n'est pas la peine de s'embêter avec.
Owner

(on pourrait gérer assez facilement le cas où page.uuid est manquant, mais alors il faudrait aussi gérer page.parent, et les occurrences dans les cellules; ça devient vite une usine à gaz - du m)

Dac si c'est compliqué on peut sûrement vivre sans (mais il ne faudrait pas que les éventuels cas où on ait à gérer ça à la main prennent plus de temps qu'avoir le code ici !)

(aussi ton commentaire est coupé, tout comme celui ici https://gitea.entrouvert.org/entrouvert/combo/pulls/25#issuecomment-1880)

> (on pourrait gérer assez facilement le cas où page.uuid est manquant, mais alors il faudrait aussi gérer page.parent, et les occurrences dans les cellules; ça devient vite une usine à gaz - du m) Dac si c'est compliqué on peut sûrement vivre sans (mais il ne faudrait pas que les éventuels cas où on ait à gérer ça à la main prennent plus de temps qu'avoir le code ici !) (aussi ton commentaire est coupé, tout comme celui ici https://gitea.entrouvert.org/entrouvert/combo/pulls/25#issuecomment-1880)
Author
Owner

Waip, gitea n'aime pas les accents circonflexes. Je vais compléter mon commentaire en essayant de retrouver ce que j'ai voulu écrire :)

Waip, gitea n'aime pas les accents circonflexes. Je vais compléter mon commentaire en essayant de retrouver ce que j'ai voulu écrire :)
lguerin force-pushed wip/67710-page-uuid from 5fc10ccf83 to 978f8a4e7f 2023-01-12 11:37:36 +01:00 Compare
lguerin force-pushed wip/67710-page-uuid from 978f8a4e7f to cfad7a20e1 2023-01-23 10:04:11 +01:00 Compare
lguerin requested review from pducroquet 2023-01-24 09:23:46 +01:00
lguerin requested review from vdeniaud 2023-02-01 09:38:21 +01:00
lguerin force-pushed wip/67710-page-uuid from cfad7a20e1 to 1248e0ff8d 2023-02-01 09:38:57 +01:00 Compare
vdeniaud approved these changes 2023-02-01 10:06:59 +01:00
vdeniaud left a comment
Owner

(moi ça me va suite aux réponses à mes questions, j'avais pas validé parce que j'avais vu une relecture en attente de Pierre)

(moi ça me va suite aux réponses à mes questions, j'avais pas validé parce que j'avais vu une relecture en attente de Pierre)
pducroquet approved these changes 2023-02-01 17:35:28 +01:00
pducroquet left a comment
Owner

J'approuve pour la migration

J'approuve pour la migration
lguerin force-pushed wip/67710-page-uuid from 1248e0ff8d to 31531e7133 2023-02-01 17:45:59 +01:00 Compare
lguerin merged commit 31531e7133 into main 2023-02-01 18:18:39 +01:00
lguerin deleted branch wip/67710-page-uuid 2023-02-01 18:18:39 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/combo#25
No description provided.