-
Notifications
You must be signed in to change notification settings - Fork 52
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
[TECH] Ajoute la domainTransaction dans le AsyncLocalStorage (PIX-13257). #9423
[TECH] Ajoute la domainTransaction dans le AsyncLocalStorage (PIX-13257). #9423
Conversation
Une fois les applications déployées, elles seront accessibles via les liens suivants :
Les variables d'environnement seront accessibles via les liens suivants : |
761f5c9
to
2eea3f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'aime le système à la ThreadLocal (s'affranchir de trimballer des parametres dans toutes les couches) !
ℹ️ Les parties barrées challengent la vision future, qui n'est pas celle que mets en place cette PR ! Ne pas en tenir compte !
Je pense qu'il faut vraiment challenger avec des connaisseurs en BDD ce nouveau système si on garde l'idée de créer à chaque requête une transaction par défaut.
Si je suis complètement en phase sur le système à la ThreadLocal (s'affranchir de trimballer des parametres dans toutes les couches), je suis + dubitatif sur le fait d'imposer une transaction systématique.
Est-ce que l'on veut pallier un manque de connaissances des développeurs (quand je dois faire un transaction, a quoi ça sert, etc.) en leur créant magiquement la chose par défaut ?
Par ailleurs, il faudrait vérifier en amont de la PROD les impacts sur l'infra, car dans un contexte de volumétrie ça peut générer des choses dures à voir sur une faible volumétrie (overhead, deadlock)
...tion/campaign-participation/infrastructure/repositories/campaign-participation-repository.js
Show resolved
Hide resolved
Sur une suggestion de @HEYGUL, j'ai rajouté un commit qui introduit un nouvel outil pour manipuler les transactions. C'est une higher order function qui permet d'améliorer la DX d'utilisation des domain transactions au niveau du usecase. Avant : import { DomainTransaction } from '../../../../shared/domain/DomainTransaction.js';
import { CampaignUniqueCodeError, UnknownCampaignId } from './../errors.js';
const updateCampaignCode = async function ({ campaignId, campaignCode, campaignAdministrationRepository }) {
await DomainTransaction.execute(async (domainTransaction) => {
const campaign = await campaignAdministrationRepository.get(campaignId, domainTransaction);
if (!campaign) {
throw new UnknownCampaignId();
}
campaign.updateFields({ code: campaignCode });
const isCodeAvailable = await campaignAdministrationRepository.isCodeAvailable({
code: campaignCode,
domainTransaction,
});
if (!isCodeAvailable) {
throw new CampaignUniqueCodeError();
}
await campaignAdministrationRepository.update(campaign, domainTransaction);
});
}; Après : import { transaction } from '../../../../shared/domain/DomainTransaction.js';
import { CampaignUniqueCodeError, UnknownCampaignId } from './../errors.js';
const updateCampaignCode = transaction(async function ({ campaignId, campaignCode, campaignAdministrationRepository }) {
const campaign = await campaignAdministrationRepository.get(campaignId);
if (!campaign) {
throw new UnknownCampaignId();
}
campaign.updateFields({ code: campaignCode });
const isCodeAvailable = await campaignAdministrationRepository.isCodeAvailable({ code: campaignCode });
if (!isCodeAvailable) {
throw new CampaignUniqueCodeError();
}
await campaignAdministrationRepository.update(campaign);
}); Sur le nommage, nous avons un doute avec @HEYGUL qui proposait aussi |
@frinyvonnick je découvre donc aujourd'hui qu'on a un DomainTransaction dans notre "couche" domaine avec du knex importé dedans 😨 Comment a été décidé cette entorse à l'inversion de dépendance ? (une petite entorse soit mais entorse quand même) |
Je vois que @dimitrilahaye tu partages mon malaise sur la question. Après discussion avec @bpetetot et @HEYGUL voici ce qui en ressort :
Je laisse les autres compléter 😄 EDIT : |
a3d38e6
to
8daf299
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM c'est cool 🌮
Il manque à mon sens de mettre à jour / amender l'ADR n°9 / n° 25 avec ce nouveau choix
8daf299
to
4fbbdc6
Compare
…rticipationFromAdmin usecase
4fbbdc6
to
726a519
Compare
🦄 Problème
Le fonctionnement de la méthode execute de DomainTransaction oblige à passer à chaque méthode de repositories appelés dans les usecases à prendre en paramètre une instance de DomainTransaction.
Le problème de cette implémentation est qu'il arrive d'oublier de passer une domainTransaction et la valeur par défaut mise au niveau des méthodes repositories avec la méthode static emptyTransaction pour simplifier les tests cache ces oublis. La conséquence est que nous avons des usecases semi-transactionnels.
🤖 Proposition
On change le fonctionnement de la méthode execute pour qu'il stocke l'instance de la DomainTransaction dans le AsyncLocalStorage.
On ajoute un méthode static getConnection qui permet de récupérer une transaction si elle existe ou l'instance knex.
On conserve la possibilité d'utiliser les DomainTransaction tel qu'on le fait aujourd'hui pour permettre de migrer progressivement.
🌈 Remarques
Voici le différence d'utilisation avant et après.
Avant :
Usecase :
Repository :
Apres
Usecase :
Repository :
TODO : @1024pix/team-prescription va devoir supprimer leur expérimentation avec l'ApplicationTransaction en faveur de la DomainTransaction.
💯 Pour tester
La CI est verte 💚