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

Connexion via LDAP : 1. Préparation #49

Merged
merged 10 commits into from
Sep 22, 2018
Merged

Connexion via LDAP : 1. Préparation #49

merged 10 commits into from
Sep 22, 2018

Conversation

prytoegrian
Copy link
Member

@prytoegrian prytoegrian commented Aug 23, 2018

J'ai trouvé la bonne approche pour la consommation de ldap par l'API. Par contre, je risquais d'avoir une grosse PR, alors je n'ai fait que la première étape, celle qui prépare le terrain en adaptant l'ancien code. Quoi qu'il en soit, on observera que le design décrit ici suffit amplement à comprendre ce que je m'apprête à faire dans l'étape réellement dédiée à la connexion LDAP via API. L'objectif de cette PR est de critiquer ce futur design.

Je suis parti du constat que ce n'était pas au client de transmettre une info à l'API pour lui dire « hey, je veux le connecteur LDAP / AD / … », c'est plutôt à l'API de comprendre dans quel contexte elle est. Dans cet optique, et puisque ce n'est PAS au contrôleur d'avoir du métier (et j'en ai marre des devs qui prétendent le contraire), j'ai créé ce qu'on appelle une fabrique. Il s'agit d'une structure à qui l'on passe des informations et qui décide toute seule de quel authentifier on a besoin. Le reste de l'application ne sait alors pas quel objet il manipule, il a juste un contrat normalisé « est-ce-que ton authentification est OK ? ».

L'inconvénient, c'est que le design AbstractFactory comme on l'appelle, rend son consommateur non testable unitairement. C'est quelque chose à noter, bien que non gravissime. C'est dans ce genre de situation qu'on va dégainer du test de service (nous devrons tout créer, rien n'est fait).

NB : j'ai trouvé un bug bien vicieux dans la gestion des requêtes SQL, donc je l'ai corrigé ici : si la valeur d'un offset de tableau était vide, alors on ne faisait pas la recherche dessus. Oui, c'est con, et je m'en excuse.
NB² : j'ai ajouté des assertions. Oui ça me tenait à cœur, Oui il est probable que j'ai utilisé quelque chose comme prétexte pour les mettre. Désolé. Les assertions sont la base de la programmation par contrat, paradigme réputé pour sa fiabilité. Il permet d'interdire tout bonnement des situations et placé aux bons endroits, facilite le débogage en vertu de la séparation consommateur / fournisseur.
En sus, son avantage est d'être sans surcoût en production, l'opération n'étant pas traitée. J'aimerais la populariser dans l'appli.

Test

L'objectif du test est de vérifier l'absence de régression sur l'authentification, ce qui est plutôt pas mal ; mais en réalité le test est peu important. Ce qui est important, c'est la review et les débats qu'elle va susciter.

@libertempo libertempo deleted a comment Aug 23, 2018
@libertempo libertempo deleted a comment Aug 23, 2018
@libertempo libertempo deleted a comment Aug 23, 2018
@prytoegrian prytoegrian changed the title Connexion via LDAP Connexion via LDAP : 1. Préparation Aug 23, 2018
@libertempo libertempo deleted a comment Sep 22, 2018
@libertempo libertempo deleted a comment Sep 22, 2018
@prytoegrian prytoegrian merged commit afc8424 into master Sep 22, 2018
@prytoegrian
Copy link
Member Author

Suite à ce qu'on a dit, je débloque le workflow.

@prytoegrian prytoegrian deleted the pry/ldapConnexion branch October 1, 2018 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant