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

[BDD] Triggers mise à jour date de création #3294

Open
ch-cbna opened this issue Dec 19, 2024 · 13 comments
Open

[BDD] Triggers mise à jour date de création #3294

ch-cbna opened this issue Dec 19, 2024 · 13 comments

Comments

@ch-cbna
Copy link
Contributor

ch-cbna commented Dec 19, 2024

J'ai une question concernant les triggers qui existent sur les champs meta_create_date des tables gn_synthese.t_sources, gn_meta.t_acquisition_frameworks, gn_meta.t_datasets, utilisateurs.t_roles et gn_synthese.synthese :
Quel est leur intérêt ?

Lorsqu'on importe des données avec des dates de création d'origine, nous souhaitons les conserver dans la base GeoNature, hors pour ce faire nous sommes obligés de désactiver ces triggers avant l'import et de les réactiver après.

@ch-cbna ch-cbna added the bug label Dec 19, 2024
@camillemonchicourt
Copy link
Member

Ce sont des dates automatiques de création dans la BDD.
Cela sert à savoir quand une donnée a été créée dans la BDD.
Cela peut notamment être utile aux API quand tu ne veux récupérer que les nouvelles données en les interrogeant par date de création ou de modification.

@camillemonchicourt
Copy link
Member

Normalement on doit pouvoir faire en sorte que les triggers ne soient déclenchés que si le champs n'est pas rempli explicitement.
Mais cela peut être nécessaire de calculer automatiquement la date d'intégration de la donnée dans la BDD, donc de ne pas récupérer les dates d'une autre BDD.

@lpofredc
Copy link
Contributor

Grand sujet, ces 2 champs font-ils partie de la donnée (au sens standard d'occurence et donc de l'historique de la donnée) ou de l'application (avec le temps, je penche de plus en plus pour cette option).

Je ne crois pas que ces valeurs (date de création/date de modification) existent dans le standard SINP (cf. https://inpn.mnhn.fr/docs-web/docs/download/421824).

Si on fait le choix de casser ce trigger pour l'intégration de données en base, c'est a réaliser après mure réflexion. A titre d'exemple, GN2PG s'appuie sur ces champs pour le dispositif de mise à jour incrémentale.

@camillemonchicourt
Copy link
Member

Oui pour moi ce sont des champs et des infos techniques, pas des infos métiers liées à un standard ou autre.

@jpm-cbna
Copy link
Contributor

Je pense que la question était plus de savoir pourquoi utilisé des triggers pour cela à la place d'une valeur par défaut de ces champs définit à NOW() ?
L'avantage de l'utilisation de la valeur par défaut c'est qu'il n'y a pas besoin de désactiver des triggers si on fournit une valeur pour le champ lors d'une requête d'insertion...
Est ce qu'il y a une raison bien précise à l'utilisation des triggers ?

@lpofredc
Copy link
Contributor

Les valeurs par défaut y sont déjà. Par contre, si tu fais un INSERT avec une valeur NULL, elle écrase la valeur NOW() définie par défaut, ce qui risque de casser certains fonctionnements applicatifs. Il faudrait peut-être juste revoir le trigger pour appliquer cette règle (extrait dessous) déjà présente dans l'UPDATE dans le CREATE, pour être certain de ne pas avoir de valeur NULL. Mais aussi d'ajouter des contraintes NOT NULL à ces champs?

if(NEW.meta_create_date IS NULL) THEN
NEW.meta_create_date = NOW();

@jpm-cbna
Copy link
Contributor

Mais aussi d'ajouter des contraintes NOT NULL à ces champs?

Justement, il me semble qu'en mettant les champs NOT NULL et avec une valeur par défaut à NOW() cela va forcer l'utilisation de la fonction NOW(). Si on essaye de mettre une valeur NULL dans le champs ou s'il n'est pas indiqué dans l'INSERT, cela devrait exécuter la fonction.

@ch-cbna va le tester. Si cela peut éviter l'utilisation de triggers, c'est surement mieux pour les performances et cela simplifie la maintenance de la base...

@ch-cbna
Copy link
Contributor Author

ch-cbna commented Dec 20, 2024

Effectivement, je confirme, en mettant les champs NOT NULL et avec une valeur par défaut à NOW() cela va forcer l'utilisation de la fonction NOW() si on envoie une valeur NULL. Du coup on n'aurait plus besoin des triggers qui forcent la valeur now() sur les champs date.

@lpofredc
Copy link
Contributor

@ch-cbna, les tests ont été fait sur les tables avec les triggers ? Car à ma connaissance, Postgresql ne gère pas nativement ce cas de figure, sans les triggers. Si tu cherches à insérer une valeur NULL dans un champ avec une contrainte NOT NULL, il te retourne une erreur pour non respect de la contrainte.

CREATE table testts(id int, ts_nullable timestamp default now(), ts_not_nullable timestamp default now() not null);

insert into testts(id, ts_nullable, ts_not_nullable) values (1,null, null);
ERROR:  null value in column "ts_not_nullable" of relation "testts" violates not-null constraint
DÉTAIL : Failing row contains (1, null, null).

@ch-cbna
Copy link
Contributor Author

ch-cbna commented Dec 20, 2024

J'ai fait le test manuellement dans Dbeaver avec la table gn_synthese.t_sources, en activant la contrainte NOT NULL et en supprimant le trigger tri_meta_dates_t_sources, puis en lançant la requête :

INSERT INTO gn_synthese.t_sources (
	name_source
)
VALUES (
	'test'
)

Je n'ai pas eu d'erreur

@lpofredc
Copy link
Contributor

Oui Oui, dans cet exemple, ça marche, parce qu'on n'insère aucune valeur dans ces champs meta_{create,update}_date.

Mais si tu fais un INSERT ou un UPDATE avec les champs meta_{create,update}_date et que la valeur de ces champs est NULL. ce qui peut arriver si tu cherches à importer ces valeurs depuis les données externes, alors tu auras une erreur. Pour moi, le risque est juste là.

@jpm-cbna
Copy link
Contributor

jpm-cbna commented Dec 20, 2024

Effectivement, la documentation de Postgresql sur la création de table l'indique pour DEFAULT default_expr:

The default expression will be used in any insert operation that does not specify a value for the column. If there is no default for a column, then the default is null.

En gros, il suffit de ne pas indiquer le champ dans l'insert/update ou alors il faut qu'il y ait une date dedans. Pour moi, c'est suffisant et les triggers ne sont pas nécessaire.
Le cas où l'on est obligé de garder le champ et d'y mettre une valeur NULL me semble rare mais je n'ai peut être pas tous les cas de figure en tête.

Par ailleurs, si tous les champs meta_create/update_date avaient pour contrainte NOT NULL et une valeur par défaut à NOW(), nous pourrions mettre simplement une valeur vide dans le champ lors d'un insert ou update. Cela ne génère pas d'erreur et déclenche bien l'utilisation de la valeur par défaut NOW().

@jpm-cbna
Copy link
Contributor

jpm-cbna commented Dec 20, 2024

Sinon, l'autre solution pour éviter trop de changement, c'est comme tu proposes @lpofredc de faire évoluer la fonction pour forcer l'utilisateur de NOW() uniquement si les champs sont NULL :

CREATE OR REPLACE FUNCTION public.fct_trg_meta_dates_change()
 RETURNS trigger
 LANGUAGE plpgsql
AS $function$
BEGIN
        IF(TG_OP = 'INSERT') THEN
                if (NEW.meta_create_date IS NULL) THEN
                        NEW.meta_create_date = NOW();
                END IF;
        ELSIF(TG_OP = 'UPDATE') THEN
                if (NEW.meta_create_date IS NULL) THEN
                    NEW.meta_update_date = NOW();
                END IF;
                
                if (NEW.meta_update_date IS NULL) THEN
                        NEW.meta_create_date = NOW();
                END IF;
        END IF;
        return NEW;
END;
$function$
;

Note : nous nous sommes rendu compte que les champs de type timestamp n'acceptent pas les chaines vides. Du coup inutile de tester le cas NULL ou vide.

Il faut également appliquer les mêmes modifications pour les fonctions utilisateurs.modify_date_insert() et utilisateurs.modify_date_update() car les triggers de la table t_roles n'utilisent pas la fonction public.fct_trg_meta_dates_change().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants