PnX-SI / GeoNature

Application de saisie et de synthèse des observations faune et flore
GNU General Public License v3.0
100 stars 102 forks source link

Alembic - SQL et lisibilité de la BDD et de ses évolutions #1574

Open camillemonchicourt opened 2 years ago

camillemonchicourt commented 2 years ago

Retours et questions sur le passage à Alembic et la lisibilité des évolutions de la BDD :

@jbrieuclp :

Comment on peut faire maintenant pour checker l'état de la BD dans Alembic, du genre si je veux checker si cette table n'a pas été virée dans une version ultérieure ?

@camillemonchicourt :

Concernant Alembic, cela n'est pas indiqué dans la doc : http://docs.geonature.fr/admin-manual.html#administration-avec-alembic ?

@jbrieuclp :

Peut-être, mais je n'ai pas envie de lancer ma machine virtuelle, ouvrir un terminal, changer de branche git pour passer sous ma branche develop, mettre à jour le lien vers l'upstream PnX-SI, faire une merge d'upstream/develop sur ma develop local, lancer l'environnement virtuel, ouvrir la doc alembic, parcourir la doc alembic, lancer des commandes alembic, voir le résultats des commandes, voir qu'elle n'a pas le résultat attendu, boire un café, recommencer avec une suivante, finalement trouver la bonne, boire un café pour fêter ça, tout ça juste pour regarder l'état de la BD sous la dernière version de l'appli et voir si une éventuelle table qui n'a aucun impact est toujours présente dans les scripts d'installes.

Donc s'il y a un moyen un peu plus facile pour trouver directos dans les fichiers visualisable dans le code sous github.com les évolutions Alembic et pouvoir faire un CTRL +F pour essayer de trouver la présence d'un élément de la BD ce serait franchement plus facile.

@TheoLechemia :

Ça repose la question de regénérer le SQL à jour à chaque release. Ça rajoute du boulot de notre côté, sauf si on l'intègre dans une intégration continue. On peut voir ça au workshop

camillemonchicourt commented 2 years ago

Oui tout cela nous a valu pas mal de questions, discussions et débats. GeoNature est très orienté BDD et a d'abord été conçu comme une BDD sur laquelle on a développé différents outils et modules.

Donc on avait différents scripts SQL organisés par schémas pour créer la BDD de GeoNature : https://github.com/PnX-SI/GeoNature/tree/master/data Idem pour TaxHub et UsersHub dont dépend GeoNature.

A chaque évolution de la BDD on faisait évoluer manuellement ces scripts de création de la BDD, ainsi que des scripts SQL de migration pour que ceux qui ont déjà la BDD puissent la mettre à jour.

C'était une approche SQL très lisible pour les administrateurs de BDD mais aussi assez lourde et fragile, potentiellement source d'erreurs, de différences entre la création de la BDD et sa mise à jour, de mises à jour partielles de la BDD, de difficultés pour les tests automatisés, des migrations dans des PR qui ne correspondaient plus aux bonnes versions au moment où on les mergeait.... Cela rendait aussi le déploiement et la mise à jour de GeoNature plus complexe pour les administrateurs qui devaient exécuter manuellement les différents scripts SQL, etc...

C'est pourquoi on avait d'abord initié un développement maison pour automatiser l’exécution de ces scripts, avant de privilégier une solution existante, plus robuste, maintenue et dédiée à cela qu'est Alembic : https://github.com/PnX-SI/GeoNature/issues/880

Avec Alembic on est passé à une méthode de gestion de la BDD et de ses évolutions, beaucoup plus orientée python. Celle-ci permet de répondre à pas mal des soucis de la méthode précédente, évoqués ci-dessus. Du moins on peut considérer qu'on est encore dans une méthode "mixte" car Alembic s'appuie encore sur les scripts SQL de création de la BDD (https://github.com/PnX-SI/GeoNature/tree/master/data) et des migrations (incluant parfois du SQL) pour faire évoluer la BDD (https://github.com/PnX-SI/GeoNature/tree/master/backend/geonature/migrations/versions)

Cependant les débats et discussions ont porté notamment sur différents aspects que tu mets en avant :

Les pistes pour répondre à cela tout en bénéficiant des évolutions et de la puissance apportée par le passage à Alembic :

DOC Alembic : http://docs.geonature.fr/admin-manual.html#administration-avec-alembic ?

bouttier commented 2 years ago

Si une table est supprimé, un grep le trouvera aussi bien dans un fichier alembic que dans un fichier migrate_xxx_to_yyy.sql, sans avoir besoin de lancer de commande. Mais comme le dit Camille, le dump du schéma à chaque release devrait aider ceux qui veulent chercher dans des fichiers SQL plutôt que dans une bdd à jour. Idéalement si on avait des modèles bien à jour dans GeoNature, on pourrait le voir par là aussi mais c’est clairement encore loin d’être le cas.

jbdesbas commented 2 years ago

Je partage la plupart des avis exprimés ici : je comprend tout à fait les avantages qu'offre Alembic pour les déploiements et mises à jours, mais les évolutions de la BDD deviennent assez opaques pour une application à l'origine "DB first" Si on a possibilité de générer automatiquement les anciens fichiers X.X.XtoY.Y.Y.sql ca me semblerait un vrai plus pour l'avenir.

camillemonchicourt commented 2 years ago

A noter que @bouttier vient de travailler sur des commandes geonature pour faciliter la lisibilité des branches et de l'état de sa BDD : https://github.com/PnX-SI/GeoNature/commit/d1a95affd9b07a72047df7fd732b289ed5ce882c

Il n'est pas prévu de regénérer les anciens fichier X.X.XtoY.Y.Y.sql, mais on a des pistes par exemple pour ordonner les fichiers de migration.

jpm-cbna commented 2 years ago

La nouvelle commande geonature db status ajouté par @bouttier sur la la branche develop permet effectivement d'y voir plus clair dans les différentes branches et migrations effectuées.

Ci-dessous un exemple qui est moins lisible que l'original car il ne contient pas la coloration syntaxique:

[geonature ✓]
  [x] ┰ f06cc80cc8ba geonature schemas 2.7.5
  [x] ┃ c0fdf2ee7f4f auto update cor_area_synthese
  [x] ┃ 7077aa76da3d bump dependencies
  [x] ┃ 2a2e5c519fd1 fix gn_synthese.get_default_nomenclature_value
  [x] ┃ 5f4c4b644844 delete cascade on  cor_dataset_territory and cor_dataset_protocol
  [x] ┃ 2aa558b1be3a add schema gn_profiles
  [x] ┃ 1eb624249f2b add default value in additionalFields bib
  [x] ┃ 7471f51011c8 change index_vm_valid_profiles_cd_ref to unique index
  [x] ┃ 9a9f4971edcd fix altitude trigger
  [x] ┃ 6f7d5549d49e delete view v_synthese_validation_forwebapp
  [x] ┣┓ dde31e76ce45 remove old profile function
  [x]  ┃ 61e46813d621 Update synthese sensitivity
  [x]  ┃ dfec5f64ac73 Fix sensitivity algorithm
  [x]  ┃ ac08dcf3f27b Do not auto-compute diffusion_level
  [x] ┃ 30edd97ae582 Remove gn_export.t_config_exports
  [x] ┣┛ 1dbc45309d6e Merge sensitivity
  [x] ┸ f350e23bee07 Add table to link bdc_status and ref_geo
[geonature-samples]
  [ ] ─ 3d0bf4ee67d1 geonature samples
[habitats ✓]
  [x] ─ 62e63cd6135d create ref_habitats schema
[habitats_inpn_data ✓]
  [x] ┰ 46e91e738845 insert inpn data in ref_habitats schema
  [x] ┸ 805442837a68 correction on habref data
[ign_bd_alti]
  [ ] ─ 1715cf31a75d Insert default French DEM (IGN 250m BD alti)
[ign_bd_alti_vector]
  [ ] ─ 87651375c2e8 Vectorize French DEM
[nomenclatures ✓]
  [x] ┰ 6015397d686a create ref_nomenclature schema 1.3.9
  [x] ┃ 11e7741319fd fix ref_nomenclatures.get_default_nomenclature_value
  [x] ┃ f8c2c8482419 fix ref_nomenclatures.get_default_nomenclature_value
  [x] ┸ b820c66d8daa fix ref_nomenclatures.get_nomenclature_label
[nomenclatures_inpn_data ✓]
  [x] ─ 96a713739fdd insert inpn data in ref_nomenclatures
[nomenclatures_taxonomie ✓]
  [x] ─ f5436084bf17 add support for taxonomy into ref_nomenclatures
[nomenclatures_taxonomie_inpn_data ✓]
  [x] ─ a763fb554ff2 insert taxonomic inpn data in ref_nomenclatures
[occhab ✓]
  [x] ─ 2984569d5df6 create occhab schema
[occhab-samples]
  [ ] ─ 21f661247023 insert occhab sample data
[occtax ✓]
  [x] ┰ 29c199e07eaa create occtax schema
  [x] ┃ addb71d8efad create occtax export view
  [x] ┃ f57107d2d0ad fix get_default_nomenclature_value
  [x] ┃ 494cb2245a43 trigger comportement
  [x] ┸ 944072911ff7 update synthese data (bug occtax trigger)
[occtax-samples]
  [ ] ─ cce08a64eb4f insert occtax sample data
[occtax-samples-test]
  [ ] ─ 2a0ab7644e1c occtax sample test
[ref_geo ✓]
  [x] ┰ 6afe74833ed0 ref_geo schema
  [x] ┃ e0ac4c9f5c0a add indexes on FK referencing l_areas.id_area
  [x] ┸ 4882d6141a41 add regions in area types
[ref_geo_fr_departments ✓]
  [x] ─ 3fdaa1805575 Insert French departments in ref_geo
[ref_geo_fr_municipalities ✓]
  [x] ─ 0dfdbfbccd63 Insert French municipalities in ref_geo
[ref_geo_fr_regions ✓]
  [x] ─ d02f4563bebe Insert French regions in ref_geo
[ref_geo_fr_regions_1970]
  [ ] ─ 05a0ae652c13 Insert French regions 1970-2016 in ref_geo
[ref_geo_inpn_grids_1 ✓]
  [x] ─ 586613e2faeb Insert INPN 1×1 grids in ref_geo
[ref_geo_inpn_grids_10 ✓]
  [x] ─ ede150d9afd9 Insert INPN 10×10 grids in ref_geo
[ref_geo_inpn_grids_5 ✓]
  [x] ─ 7d6e98441e4c Insert INPN 5×5 grids in ref_geo
[ref_sensitivity_inpn]
  [ ] ─ 7dfd0a813f86 Insert INPN rules in sensitivity referential
[sql_utils ✓]
  [x] ─ 3842a6d800a0 Add public shared functions
[taxhub]
  [ ] ─ fa5a90853c45 taxhub
[taxhub-admin]
  [ ] ─ 3fe8c07741be taxhub
[taxonomie ✓]
  [x] ┰ 9c2c0254aadc create taxonomie schema version 1.8.1
  [x] ┃ 7540702c6407 cd_ref utility functions
  [x] ┃ 98035939bc0d find_all_taxons_parents
  [x] ┃ c93cbb35cfe4 set default value for id_liste
  [x] ┸ 4fb7e197d241 create taxonomie.v_bdc_status view
[taxonomie_attributes_example]
  [ ] ─ aa7533601e41 add attributes exemple to taxonomie
[taxonomie_inpn_data ✓]
  [x] ─ f61f95136ec3 insert inpn data in taxonomie schema
[taxonomie_taxons_example]
  [ ] ─ 8222017dc3f6 add taxons exemple to taxonomie
[usershub]
  [ ] ┰ 9445a69f2bed UsersHub
  [ ] ┸ 6ec215fe023e upgrade utilisateurs schema
[usershub-samples]
  [ ] ─ f63a8f44c969 UsersHub samples data
[utilisateurs ✓]
  [x] ┰ fa35dfe5ff27 utilisateurs schema 1.4.7 (usershub 2.1.3)
  [x] ┃ 830cc8f4daef add additional_data field to bib_organismes
  [x] ┃ 5b334b77f5f5 fix v_roleslist_forall_applications
  [x] ┃ 951b8270a1cf add unique constraint on bib_organismes.uuid_organisme
  [x] ┸ 10e87bc144cd get_id_role_by_name()
[utilisateurs-samples ✓]
  [x] ─ 72f227e37bdf utilisateurs sample data

Avec le résultat ci-dessus, il devient plus facile de demander le SQL correspondant à la migration de la révision 7471f51011c8 à la révision 6f7d5549d49e pour la branche geonature grâce à la commande suivante :

geonature db upgrade 7471f51011c8:6f7d5549d49e -x local-srid="4326" --sql > migration.sql

Le fichier SQL migration.sql contiendra alors:

BEGIN;

-- Running upgrade 7471f51011c8 -> 9a9f4971edcd

CREATE OR REPLACE FUNCTION ref_geo.fct_trg_calculate_alt_minmax()
        RETURNS trigger
        LANGUAGE plpgsql
        AS $function$
        DECLARE
            the4326geomcol text := quote_ident(TG_ARGV[0]);
        thelocalsrid int;
        BEGIN
            -- si c'est un insert et que l'altitude min ou max est null -> on calcule
            IF (TG_OP = 'INSERT' and (new.altitude_min IS NULL or new.altitude_max IS NULL)) THEN
                --récupérer le srid local
                SELECT INTO thelocalsrid parameter_value::int FROM gn_commons.t_parameters WHERE parameter_name = 'local_srid';
                --Calcul de l'altitude

            SELECT (ref_geo.fct_get_altitude_intersection(st_transform(hstore(NEW)-> the4326geomcol,thelocalsrid))).*  INTO NEW.altitude_min, NEW.altitude_max;
            -- si c'est un update et que la geom a changé
            -- on vérifie que les altitude ne sont pas null
            -- OU si les altitudes ont changé, si oui =  elles ont déjà été calculés - on ne relance pas le calcul
        ELSIF (
                TG_OP = 'UPDATE' 
                AND NOT public.ST_EQUALS(hstore(OLD)-> the4326geomcol, hstore(NEW)-> the4326geomcol)
                and (new.altitude_min = old.altitude_max or new.altitude_max = old.altitude_max)
                and not(new.altitude_min is null or new.altitude_max is null)
                ) then
            --IF (new.altitude_min is null or new.altitude_max is null) OR (NOT OLD.altitude_min = NEW.altitude_min or NOT OLD.altitude_max = OLD.altitude_max) THEN
            --récupérer le srid local
            SELECT INTO thelocalsrid parameter_value::int FROM gn_commons.t_parameters WHERE parameter_name = 'local_srid';
                --Calcul de l'altitude
                SELECT (ref_geo.fct_get_altitude_intersection(st_transform(hstore(NEW)-> the4326geomcol,thelocalsrid))).*  INTO NEW.altitude_min, NEW.altitude_max;
            --end IF;
            --else
        END IF; 
        RETURN NEW;
        END;
        $function$
        ;;

UPDATE alembic_version SET version_num='9a9f4971edcd' WHERE alembic_version.version_num = '7471f51011c8';

-- Running upgrade 9a9f4971edcd -> 6f7d5549d49e

DROP VIEW gn_commons.v_synthese_validation_forwebapp;;

UPDATE alembic_version SET version_num='6f7d5549d49e' WHERE alembic_version.version_num = '9a9f4971edcd';

COMMIT;

J'ai aussi réussi à récupérer tout le SQL d'une branche comme ceci:

geonature db upgrade nomenclatures_inpn_data --sql > migration.sql
camillemonchicourt commented 1 year ago

Intégration de la commande geonature db status dans la 2.10.0, permettant de lister les migrations Alembic, leurs dépendances et identifier celles qui ont été appliquées ou non.

andriacap commented 10 months ago

Salut tout le monde,

Petite réflexion concernant l'utilisation d'alembic notamment pour la gestion de vues , fonction :

A l'heure actuelle , il y a pas mal de migrations alembic qui sont réalisées en écrivant du texte sql directement dans les fonctions downgrade et upgrade Voir exemple : https://github.com/PnX-SI/GeoNature/blob/22b577a5809a177d6ff75d0bb838dc8060d7a304/backend/geonature/migrations/versions/6f7d5549d49e_delete_view_v_synthese_validation_forwebapp.py#L19-L112

La documentation d'alembic propose dans sa documentation une gestion des vues , procédures sql. Voir lien ici : https://alembic.sqlalchemy.org/en/latest/cookbook.html#the-replaceable-object-structure

L'autre point concerne l'écriture en dur pour la suppression / creation de vues sql dans chaque fichier de migration alembic ou bien d'appeler un fichier sql créé à part et qui serait appelé dans les fichiers de migrations alembic.

Concernant les opérations custom proposées par alembic ça donnerait un truc comme ça :

Un fichier où l'on créé les opérations alembic custom

fichier alembic_utils ```py from alembic.operations import Operations, MigrateOperation class ReplaceableObject: def __init__(self, name, sqltext): self.name = name self.sqltext = sqltext class ReversibleOp(MigrateOperation): def __init__(self, target): self.target = target @classmethod def invoke_for_target(cls, operations, target): op = cls(target) return operations.invoke(op) def reverse(self): raise NotImplementedError() @classmethod def _get_object_from_version(cls, operations, ident): version, objname = ident.split(".") module = operations.get_context().script.get_revision(version).module obj = getattr(module, objname) return obj @classmethod def replace(cls, operations, target, replaces=None, replace_with=None): if replaces: old_obj = cls._get_object_from_version(operations, replaces) drop_old = cls(old_obj).reverse() create_new = cls(target) elif replace_with: old_obj = cls._get_object_from_version(operations, replace_with) drop_old = cls(target).reverse() create_new = cls(old_obj) else: raise TypeError("replaces or replace_with is required") operations.invoke(drop_old) operations.invoke(create_new) @Operations.register_operation("create_view", "invoke_for_target") @Operations.register_operation("replace_view", "replace") class CreateViewOp(ReversibleOp): def reverse(self): return DropViewOp(self.target) @Operations.register_operation("drop_view", "invoke_for_target") class DropViewOp(ReversibleOp): def reverse(self): return CreateViewOp(self.target) @Operations.implementation_for(CreateViewOp) def create_view(operations, operation): operations.execute("CREATE VIEW %s AS %s" % (operation.target.name, operation.target.sqltext)) @Operations.implementation_for(DropViewOp) def drop_view(operations, operation): operations.execute("DROP VIEW %s" % operation.target.name) ```

Et ensuite on utilise de cette manière dans les fichiers de migrations alembic

1ere revision alembic qui modifie la vue initiale ```py """add id_module to v_synthese_for_web_app Revision ID: 446e902a14e7 Revises: f1dd984bff97 Create Date: 2023-09-25 10:09:39.126531 """ import importlib from alembic import op from sqlalchemy.sql import text from geonature.utils import alembic_utils # revision identifiers, used by Alembic. revision = "446e902a14e7" down_revision = "f1dd984bff97" branch_labels = None depends_on = None view_name = "gn_synthese.v_synthese_for_web_app" path_synthese = "geonature.migrations.data.core.gn_synthese" init_filename = "initial_v_synthese_for_web_app_v1.0.0.sql" sql_text = text(importlib.resources.read_text(path_synthese, init_filename)) v_synthese_for_web_app_init = alembic_utils.ReplaceableObject(view_name, sql_text) filename = "v_synthese_for_web_app_add_id_module_v1.0.1.sql" sql_text = text(importlib.resources.read_text(path_synthese, filename)) v_synthese_for_web_app = alembic_utils.ReplaceableObject(view_name, sql_text) def upgrade(): op.drop_view(v_synthese_for_web_app_init) op.create_view(v_synthese_for_web_app) def downgrade(): op.drop_view(v_synthese_for_web_app) op.create_view(v_synthese_for_web_app_init) ```
2nde revision alembic qui modifie une nouvelle fois la vue de la précédente revision ```py """add_column_group_inpn_to_v_synthese_for_web_app Revision ID: d99a7c22cc3c Revises: 446e902a14e7 Create Date: 2023-11-17 14:53:42.138762 """ import importlib from alembic import op from sqlalchemy.sql import text from geonature.utils import alembic_utils # revision identifiers, used by Alembic. revision = "d99a7c22cc3c" down_revision = "446e902a14e7" branch_labels = None depends_on = ("c4415009f164",) # Taxref v15 db structure view_name = "gn_synthese.v_synthese_for_web_app" path_synthese = "geonature.migrations.data.core.gn_synthese" filename = "v_synthese_for_web_app_add_group_inpn_v1.0.2.sql" sql_text = text(importlib.resources.read_text(path_synthese, filename)) v_synthese_for_web_app = alembic_utils.ReplaceableObject(view_name, sql_text) def upgrade(): # on fait référence ici à la vue de la revision précédente en spécifiant l'identifiant de la revision op.replace_view(v_synthese_for_web_app, replaces="446e902a14e7.v_synthese_for_web_app") def downgrade(): op.replace_view(v_synthese_for_web_app, replace_with="446e902a14e7.v_synthese_for_web_app") ```

A discuter , réflechir concernant une bonne pratique à mettre en place pour la gestion d'alembic Merci d'avance pour vos retours :) !

camillemonchicourt commented 9 months ago

Comme j'indiquai dans la PR où on a parlé du sujet (https://github.com/PnX-SI/GeoNature/pull/2637#issuecomment-1818422558), je ne ferai pas un fichier SQL par changement, au contraire. Je ferai un seul fichier SQL avec toutes les vues, fonctions et triggers de chaque module (un pour Synthèse, un pour Occtax...) et que chaque fois qu'une vue, fonction ou trigger évolue, on modifie ce fichier et on le relance depuis une migration (ou alors on relance tout le temps ce fichier à chaque migration, comme on fait dans Geotrek-admin). Cela centraliserait les vues, fonctions et triggers de chaque module et faciliterait grandement le suivi et la lisibilité de leurs évolutions, le diff permettant de voir ce qui a changé, contrairement à actuellement ou on réécrit complètement la vue dans les fichiers de migration, sans pouvoir identifier facilement les modifications).

jacquesfize commented 9 months ago

Plutôt intéressé par le fait de centraliser les triggers, les fonctions et les vues (TFV pour la suite) dans des fichiers uniques. Toutefois, il faudra accompagnez cela d'une commande geonature db install_tfv pour installer ces derniers sur la DB. De mon côté, je trouve qu'il faudrait éviter le plus possible d'exécuter du SQL brut mais de passer par les opérateurs fournis par alembic qui réponde à une grande majorité de besoins : https://alembic.sqlalchemy.org/en/latest/ops.html#alembic.operations.Operations (sauf trigger, vue et fonction). De plus, sachant que les fichiers de révisions d'alembic concernerait uniquement les changements de modèles si l'on externalise les TFV, on devrait profiter de la génération automatique de révisions proposée par flask-migrate (utiliser dans geonature).