failing test

[JanTvrdik]

may or may not be related to #171

[hrach]

Co toto presne znamena? Co se timto checkuje?

[JanTvrdik]

(wasLoaded má hodnotu true) právě tehdy, když (collection měla nenulovou hodnotu, ale byla invalidována = nastavena na null)

wasLoaded se používá pouze na to, aby getEntitiesForPersistence vedělo, zda si má nechat postavit (pokud tomu ještě tak není) celou kolekci (protože ví, že to umíme udělat bez dalšího dotazu do DB, neboť všechny entity už jsou v paměti) nebo projít jenom to, co máme teď v paměti (říkejme tomu třeba partial kolekce).

[JanTvrdik]

Jinak obecně – ty testy by měli pokrývat ideálně všechny možné kombinace volání add. Tj. pokud se něco zdá zbytečné, tak to zkus zakomentovat / zjednodušit a pusť testy a některý by se měl vždycky rozbít. Pokud ne, tak je to bug v testech nebo v implementaci. Pokaždé, když jsem na něco takového sám narazil, tak jsem buď opravil implementaci nebo testy.

[JanTvrdik]

Jinak další plán vidím zhruba takto:

1. Zajistit, aby procházely testy testFetchExistingA a testRemoveA.
2. Přidat hromadu dalších testů na remove a opravit nalezené chyby.
3. Přidat hromadu dalších testů na kombinace add a remove a opravit nalezené chyby.
4. Zkusit to celé zrefaktorovat a nerozbít přitom testy.

[hrach]

Takze sem to cely prosel a pochopil, co resis a o co se snazis.

1. prijdem mi, ze enorme narusta komplexita kodu za cenu nejakych optimalizaci, ze po persit vracim porad jen jednu entitu, misto abych zacetl znovu celou kolekci. Ten usecase, ze neco persistuji a potom znovu ctu a znovu persistuji ji neuveritelne maly. Optimalizace ma dle me probihat na urovno volani nekaskadoveho persist. Aktualni array-init je ukazkou, jak je to naprosto spatne. Nerikam, ze by se mi dane chovani nelibilo, ale nevidim zadnou pridanou hodnotu v tom jeste ukladat specialne $added a $removed.
2. nevim, ale resi aktulni commity v PR nejaky usecase, kdyby do user-land kodu by byly spatne vraceny entity v relationship? Pokud ano, urcite bych to oddelil samostatne, aspon commit.
3. testFetchExistingA je zajimavy problem, edge-case, ktery bych rad resil, na druhou stranu jeho reseni nemusi byt uplne trivialni a vykonostne optimalni.
4. testRemoveA - jasny, to je relativne easy fix, imo dost unrelated :-)

[JanTvrdik]

Nevidím to primárně jako řešení nějakých optimalizací, ale jako řešení toho, že ORM teď „náhodně“ ztrácí data a nepersistuje data, co má persistovat. Dejme tomu, že jsem ochoten akceptovat, že některé kombinace nebudeme podporovat, ale v tom případě musí být jasně definované a vyhazovat výjimku, tj. nesmí selhat potichu, jak se to děje doposud.

Nicméně myslím, že je neakceptovatelné, aby $post->likes->add(new Like($user)); persist($post) načetlo celou kolekci likes do paměti – může obsahovat klidně miliony entit.

Předpokládejme na chvíli, že bychom nechali $post->likes->add(new Like($user)); persist($post) načíst celou kolekci likes, tj. bylo by na programátorovi, aby zavolal persist($like, false), protože samotné persist($like) by udělalo to samé jako persist($post). Nicméně i v tomto scénáři se nevyhneš poli added, protože při sekvenci getIterator-add-persist-add-getIterator dostaneš (tiše) špatný výsledek – v kolekci bude chybět entita přidaná prvním volání add, což je neakceptovatelné.

resi aktulni commity v PR nejaky usecase, kdyby do user-land kodu by byly spatne vraceny entity v relationship?

Ano, stačí si pustit ty testy nad aktuálním masterem a zakomentovat ty asserty, které se netýkají výsledků getIterator.

testFetchExistingA je zajimavy problem

Imho bys měl právě tu entitu přidat do added aneb pořád to dokola – imho prostě potřebuješ mít podporu pro partial collection – možnost říct, že nějaká entita je v té kolekci, ale přitom tu kolekci nutně celou nenačítat.

testRemoveA - jasny, to je relativne easy fix

Tak to oprav, já se na to díval a nevím, co s tím moc dělat. Až se to tady rebasne, tak se budeme moci podívat na ten remove pořádně. Obecně remove kódu vůbec nevěřím, protože na něj nejsou testy intuitivně tuším, že to nebude vždycky fungovat.

[hrach]

Cela ma predstava je zalozena na tom, ze v userlandu to bude fungovat samozrejme spolehlive. Nicmene to, co jsem si namyslel, by nefungovalo v priade add->persist->add->persist, kdy by to nezapersistovalo zmeny v prvni entity. Takze beru zpet a toto asi teda bude nutne udelat, jak si to udelal ty.

Nenacitat celou kolekci likes - samozrejme souhlas, to se ani puvodne nedelo.

Test fetchExistingA z tveho posledniho komentare moc nechapu co tim chces rict, problem mi prijde trochu nekde jinde.

TestRemove - jojo, to asi opravim. Snad to stihnu zitra.

[JanTvrdik]

Test fetchExistingA z tveho posledniho komentare moc nechapu co tim chces rict

Ten test dělá následující – načte entitu Book a inicializuje na ní property author, takže ten book má referenci a ví o entitě author. Problém je, že ten vztah je asymetrický = kniha ví o svém autorovi, ale autor neví o tom, že knihu napsal. Správně by ORM mělo imho udržovat vždy symetrii relací, tj. konkrétně v tomhle případě by měla instance knihy skončit v $author->books->added.

[hrach]

Super, tak to jsem te pochopil.

Jeste premyslim, k cemu je presne $removed? Nestaci jen to added k drzeni partial kolekce?

[hrach]

s tim testRemoveA nevim. kdyz ten test udelam nad aktualnim radkem, tak to dalsi query nespousti, tzn. imo spis si spatne detekoval, co zpusobuje ten dotaz navic: 112a33e Ja sem samozrejme predtim myslel, ze vim kde je chyba, ale tam nakonec nebyla, a obecne jsem ji v tom danym usecase nenasel.

[JanTvrdik]

Jeste premyslim, k cemu je presne $removed?

Nikdo neví, nejsou na to testy. Ale imho selže sekvence getIterator-remove-persist-remove-getIterator (vrátí i entitu odstraněnou prvním remove).

kdyz ten test udelam nad aktualnim radkem, tak to dalsi query nespousti

Je to takhle nebo dokonce takhle jasnější?

[hrach]

Jeste premyslim, k cemu je presne $removed?

Nikdo neví, nejsou na to testy. Ale imho selže sekvence getIterator-remove-persist-remove-getIterator (vrátí i entitu odstraněnou prvním remove).

Protoze persist nulluje kolekce, mela byt dalsi getIterator volani ji nacist z db, cimz dostane korektni stav v db (bez prvniho remove) a unsetne z ni nepersistovanou druhou removed entitu, ne?

kdyz ten test udelam nad aktualnim radkem, tak to dalsi query nespousti

je to takhle nebo dokonce takhle jasnější?

To prvni tady:
Aha, to uz zacina byt jasnejsi, castecne je to #82 - two pass, castecne na to bug otevreny neni - pro one pass.

[JanTvrdik]

Protoze persist nulluje kolekce,

Flush resetuje relationship cache, persist afaik nikoliv.

[hrach]

@JanTvrdik do you have idea why this is needed and why you have put it here? I see some tests fail if I removed so, something has changed in this pr to require this.