search

LuccaSA / RestDrivenDomain

8 stars 1 forks source link

collection queries should be ordered by default #153

Open rducom opened 6 years ago

rducom commented 6 years ago 
warn: Microsoft.EntityFrameworkCore.Query[10102]
      Query: '(from Account t in DbSet<Account> where True where True &&  (from string <generated>_1 in List<strin...' uses a row limiting operation (Skip/Take) without OrderBy which may lead to unpredictable results.
nfaugout-lucca commented 6 years ago

OK, je vois, on limite le nb de résultats mais on n'ordonne pas, ce qui n'est pas logique.

On pourrait orderby id desc ça me semble le plus pertinent pour la majorité des cas (sauf si on a des GUID comme identifiants)

nfaugout-lucca commented 5 years ago

@rducom je n'arrive pas à reproduire le pb même avec un test Web avec une base EF InMemory.

Ca sous entend que InMemory n'est pas assez proche de la "réalité" on dirait.

Est-ce qu'on pourrait passer sous SQLLite ou une autre alternative qui permettrait à la fois d'avoir la souplesse de InMemory de EF tout en étant encore plus proche de ce qui peut se passer en prod sur une base SQL Server ?

rducom commented 5 years ago

Oui SQLite c'est tout à fait possible. Pour autant c'est loin d'être garanti qu'on chope le même message d'erreur. On peut aussi utiliser localDB (SQL express est installé sur appveyor, suffit de le provisionner dans le yaml). Pour récupérer le message d'erreur, j'ai une astuce pour Hooker les logs de xunit. A noter que inmemory et d'autres providers sont complémentaires : il suffit de piloter la variable "environnement" et switcher selon la valeur. Ce serait d'ailleurs très pertinents de démultiplier les tests d'intégration sur l'ensemble des providers disponibles.

nfaugout-lucca commented 5 years ago

J'ai suivi ce tuto et installé Sqlite, ça m'a l'air pas mal : https://garywoodfine.com/entity-framework-core-memory-testing-database/

J'ai plein d'erreurs, je vais essayer d'aller au bout.

Et en effet faudra qu'on fasse une passe : inMemory, Relational, etc..

nfaugout-lucca commented 5 years ago

Je suis passé sur SQLite mais ce n'est pas mieux, tous les tests sont verts alors qu'ils devraient planter à cause du paging à 10 éléments sachant qu'il n'y a pas de order by.

Je vais chercher un SQLExpress ou autre.

rducom commented 5 years ago

Attention, le message que je remonte est un warning, pas un plantage.

warn: Microsoft.EntityFrameworkCore.Query[10102]

Pour choper ce warn, y'a moyen de hooker les logs de Xunit sur un logger, mais je pense que c'est overkill comme assertion pour un TU. Checker qu'il existe une clause order by dans l'expression sera plus simple je pense.

nfaugout-lucca commented 5 years ago

OK je vois. Je passe sur localdb, ça a l'air de faire le job

nfaugout-lucca commented 5 years ago

Wow, localdb qui marche enfin, c'est beau de voir chaque test jouer de façon isolée sur SA petite base de données ;)

ssms_2018-10-03_08-53-43

Par contre je ne reproduis toujours pas le pb de paging sans orderby default ! Mais bon je devrais maintenant y arriver !

nfaugout-lucca commented 5 years ago

Bon, au départ je voulais resolve cette issue, mais après avoir fait tout ça : https://github.com/LuccaSA/RestDrivenDomain/pull/174, je me rends compte que ça n'a rien à voir !

L'issue ici suggère d'ajouter un OrderBy par défaut dans les Repo. Le pb c'est qu'il faudrait avoir l'Id de l'entité sous la main, ce qui n'est pas le cas puisque les Repo n'ont pas la contrainte TKey, étant donné qu'ils peuvent travailler sur des classes qui n'ont pas d'ID.

A mon avis ce n'est pas très grave que le Skip/Take prenne 10 entrées "au hasard", car la plupart du temps un consommateur mettra explicitement un orderby dans ses queries pour obtenir ce qu'il veut vraiment.

Donc je suggère qu'on close cette issue.