Open Poltuu opened 6 years ago
Merci pour ce résumé, ça me servira au moins pour le CIR 2018 ;)
Si je peux ajouter des pistes pour pencher pour l'une ou l'autre solution, les voici :
les fields n'ont pas forcément vocation à continuer d'exister. En effet, si on arrive à correctement découpler nos API, à les organiser en agrégats (une API principale avec des sous APIs), et si on arrive à gérer cette complexité du nombre d'appels HTTP à faire, on pourrait fixer dans RDD v4 une règle qui dit : un appel sur une collection NE retourne QUE id, name, url
, un appel sur une ressource retourne toujours tous ses fields. Ainsi, on n'aurait plus de réponse partielle. Cela va de pair avec la volonté de ne plus faire des réponses à N niveaux de profondeur, mais plutôt exposer les sous ressources via id, name, url
également. Charge à l'appelant de faire d'autres appels pour explorer les API à partir d'un premier appel.
le point précédent fait également sauter le point sur les droits de visibilité. Car dans une telle optique, si tu peux voir une ressource, alors tu peux voir tous ses fields. Par ex pour les ED sur l'API des users, plutôt que de tout exposer sous l'API des users, ils seraient exposées sous une sous API, et du coup si tu n'as pas accès à la sous API, tu ne verras rien. Après je ne rentre pas dans le détail des sections Poplee, car c'est tellement spécifique dans notre panel d'APIs que ça ne me choquerait pas de devoir faire un gros override de la sérialisation native de RDD pour supporter un tel besoin.
Je pense que c'est trop tôt pour mettre ces contraintes dans RDD v3 et qu'on peut continuer à en discuter pour RDD v4 par ex. Cela correspondra sans doute avec du tooling pour la mise en cache HTTP & co, qui va de pair avec le fait de demander aux applis de faire x10 en appels HTTP !
@Poltuu je reprends tes points :
la sérialization trackée (guidée par les fields) n'est pas un standard, et newtonsoft n'a pas été développé dans cette direction, avec cette idée à l'esprit.
J'utilise sur https://github.com/LuccaSA/RestDrivenDomain/pull/141 la propriété ShouldSerialize de chaque JsonProperty, de façon à piloter si oui ou non une propriété doit être sérialisée. Ceci permet à la fois de ne pas altérer la gestion de cache de NewtonSoft.Json, et de piloter la serialization trackée par fields.
la sérialization d'un objet est censé exposer l'objet entrant, pas le modifier ou l’utiliser, ce qui rend certaines fonctionnalités disponibles aujourd’hui compliquées ou impossibles.
Peux-tu liste les fonctionnalités auxquelles tu pense ?
les hautes performances de newtonsoft sont liées à une mise en cache de nombreux aspects de la serialization. Cela se marie mal avec le besoin de résultats dynamiques. / une très grande partie des performances proposées par newtonsoft est jeté à la poubelle par cette ligne
Comme dit plus haut, l'approche sur https://github.com/LuccaSA/RestDrivenDomain/pull/141 n'altère pas la mise en cache de Newtonsoft.
les possibilités d'override sont donc plus limitées quand on les couple avec les fields.
C'est pas clair, peux-tu développer ?
l'extensibilité d'un tel système est extrêmement faible. Par exemple, après avoir développé toutes les fonctionnalités, j'ai essayé de développer la dernière, à savoir permettre l'affichage de toutes les propriétés d'un objet si son type mère est un type reconnu par RDD comme un type exposé
Si je comprends biens, là il s'agit de piloter les fields par default. Dans ce cas, cette problématique est externe à la serialization. Je vais créer une issue et proposer une API à ce sujet dans la journée.
Globalement, l'approche que j'ai adoptée sur https://github.com/LuccaSA/RestDrivenDomain/pull/141 est de modifier au minimum le standard, de façon à permettre le minimum d'overhead, et d'accéder aux features natives de Newtonsoft.Json.
L'aspect qui m'étonne le plus dans ton message concerne les features de customisation de la serialization. Je ne vois pas ce que la serialization Rdd peut faire actuellement et qu'un JsonConverter ne peut pas faire.
J'aimerais ici ouvrir le sujet de la sérialization RDD, avec notamment l'idée de modifier en profondeur l'existant (basé notamment sur les
ISerializer
,IRddSerializer
etISerializerProvider
) versus la sérialization native de aspnet core, à savoir newtonsoft.Cette question est soulevée par raph et la PR https://github.com/LuccaSA/RestDrivenDomain/pull/141/files. J'ai eu pour mission de découper cette PR en deux morceaux, à savoir la partie sérialization vs le reste. J'y travaille depuis 10 jours, et j'ai une version qui "marche". Arrivée à ce point, il m'a semblé nécessaire de discuter de ce que j'ai appris lors de ce développement, et ce que je pense des implications d'une telle modification afin que tout le monde soit bien d'accord.
Accrochez vos ceintures, j'ai beaucoup de choses à dire.
Méthode existante : description, avantages et inconvénients
Description
La méthode existante concernant la sérialization est la suivante:
Metadata
.SerializerProvider
(injecté) de trouver le bonISerializer
, et ce dernier "serialize" l'objet.object
(un dictionnaire de propriétés pour un objet, un array pour une collection), populé à partir du résultat de l'action, mais ne contenant que les propriétés désirés par les fields. Ce n'est donc pas de la serialization à proprement parler.Avantages
Inconvénients
Méthode suggérée: description, avantages et inconvénients
La méthode suggérée consiste en l'application des filtres de visibilité demandés par les fields directement au moment de la serialization par newtonsoft. Je vais entrer dans les détails techniques de ce qu'il se passerait, sachant que je ne décris ici ni l'implémentation choisie par raph, ni la mienne, mais l'essentiel de ce qu'une telle implémentation doit faire:
IContractResolver
de ces paramètres est un objet custom s'appuyant sur les fields de la requête.jsontextwriter
utilisé est unjsontextwriter
custom, trackant la position en train d'être sérializé au sein de l’objet sérializé. A noter que lejsontextwriter.Path
est incorrect et ne peut être utilisé pour notre besoin.IContractResolver
implémente sur toutes les propriétés retrouvées la méthodeShouldSerialize
par une méthode s'appuyant sur cejsontextwriter
et les fields courant, afin de savoir si la propriété courant doit être sérializée ou non.Avantages
Inconvénients
Implémentation
J'ai implémenté une version sur cette branche https://github.com/LuccaSA/RestDrivenDomain/tree/newtonsoft_serializaer qui marche complètement, càd qui est quasi iso-comportement avec l'existant, car elle prend en compte:
Je pense que ma version est plus aboutie que celle de raph, car elle va plus loin dans le support de fonctionnalités existantes. Je l'ai développé en me souciant des possibilités d'extension du système, notamment pour obtenir une couverture similaire de l'existant. Par contre, elle ne permet pas le pilotage de la serialization via contentype negociation.
Néanmoins, après avoir travaillé 10 jours dessus et essayé de très très très nombreuses versions, je ne suis pas satisfait du résultat final, pour des raisons diverses:
ISerializer
est leJsonConverter
. Celui-ci propose en effet une méthodeWriteJson(JsonWriter writer, object value, JsonSerializer serializer)
permettant d'overrider la serialization d'un objet choisi. Il n'est pas impossible de proposer unRddJsonConverter
, capable de proposer les fields à appliquer à l'objet en question, ce que je n'ai pas gardé dans la version finale. Néanmoins:JsonConverter
sont enregistrés au fur et à mesure et sauvegardés dans une liste. Pour savoir quel converter s'applique à tel objet, unFirstOrDefault
est appliqué. Cela signifie que leEntityBaseRddConverter
doit impérativement être enregistré en dernier, sinon il passera avant d'autres overrides plus précis, ce qui ne peut pas être garanti (sauf truc moche spécifique non scalable). C'est un problème d'ordre général sur cet aspect du choix du converter (je sais que des solutions existent), lesJsonConverter
ayant été conçus pour sérializer un type bien précis, et pas vraiment autre chose.JsonConverter
pour empêcher les comportements statics/mis en cache de newtonsoft de se mettre en place, impactant ainsi négativement les perfs. On se retrouve donc à instancier ces objets à chaque demande, car ils dépendent, en gros, des fields. J'ai essayé une version qui ne dépendrait que d'unIServiceProvider
, mais je n'ai jamais réussi à la faire marcher, ni à comprendre pourquoi elle ne marchait pas (qq chose était mis en cache, et sur deux tests d'affilées, le second plantait toujours).JsonConverter
ne couvrent pas tous les besoins liés à un type. Si je souhaite par exemple rajouter une propriété calculée à un type, je dois faire ça au niveau du contractresolver. Ce n'est pas forcèment contre-intuitif (encore que), mais ça pose un problème, car nous avons besoin d'avoir la main sur le contractresolver. Il ne semble pas possible de permettre/demander l'override de notreSelectiveContractResolver
pour ce besoin, notamment car cet objet dépend des fields, qui sont non injectables.IRddConverter
destiné à un type précis, équivalent duISerializer
, qui dispatcherait les comportements aux bons endroits Newtonsoft. Mais on se retrouve donc à la case départ concernant l'intérêt d'utiliser un standard, car les dévs manipuleront des objets RDD au lieu du truc natif.IServiceProvider
comme dépendance (qui aurait du permettre d'avoir des singletons sur ces objets, ni l'enregistrement des dépendances en transient/scope/singleton (j'ai tout essayé, dans le doute). Si qqun a une idée, je suis preneur. De manière générale, on se bat contre tous les bénéfices d'une librairie static, ce qui n'est pas agréable ou recommandé.IExpressionTree
que de réussir à développer ce genre de feature avec le système natif. Or, cela est un besoin recevable, qu'un consommateur pourrait répliquer (qu'un type ne sois pas sensible aux fields)Conclusion
Face à ce constat, je pense qu'en l'état, changer de système de sérialization est une mauvaise idée. Dans l'avenir, ce système peut aller dans deux directions opposées pour résoudre les vrais problèmes mentionnés plus haut:
Comment me faire changer d'avis
2 méthodes:
ou
contractresolver
(SelectiveContractResolver ligne 27)