pharo-rdbms / glorp

Generic Lightweight Object Relational Persistence
MIT License
24 stars 24 forks source link

alsoFetch: (a ToManyRelation) now also resolves the collection proxy #115

Closed ironirc closed 1 year ago

ironirc commented 1 year ago

As requested by astares, a new PR. I just applied the code, without any further testing.

codecov[bot] commented 1 year ago

Codecov Report

Merging #115 (d3e84d5) into master (94e1168) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #115    +/-   ##
========================================
  Coverage   77.76%   77.76%            
========================================
  Files         469      470     +1     
  Lines       50179    50324   +145     
========================================
+ Hits        39020    39136   +116     
- Misses      11159    11188    +29     
Impacted Files Coverage Δ
.../GlorpReadingPersonWithEmailAddressesTest.class.st 100.00% <100.00%> (ø)
...orpReadingPersonWithoutEmailAddressesTest.class.st 100.00% <100.00%> (ø)
src/Glorp/GlorpAttributeModel.class.st 97.81% <100.00%> (-0.07%) :arrow_down:
src/Glorp/GlorpCursoredStream.class.st 75.35% <100.00%> (ø)

... and 35 files with indirect coverage changes

astares commented 1 year ago

For the records: this is a substitute for https://github.com/pharo-rdbms/glorp/pull/8

and now checks have passed without merge conflict.

But having a better description and a tests for the actual situation that it solves would be a plus and a safe way forward. Would that be possible to provide?

Maybe also users, who use Glorp in production systems (like @gcotelli) can try the fix with their code/data/system before we merge to confirm it has no side effects.

ironirc commented 1 year ago

I found my original post on discord about this: https://discord.com/channels/223421264751099906/300255715337961474/562960614536904709 This was the content of the original post: Hi, I've got a question about Glorp "alsoFetch:" example:

((SimpleQuery read: Invoice)
    alsoFetch: [:each | each invoiceLines asOuterJoin ]) executeIn: session.

For the invoices that don't have invoiceLines, the ToMany proxy invoiceLines doesn't get resolved. Can someone confirm this?

gcotelli commented 1 year ago

I've run our own test suite against these changes and all tests are still passing. As @astares suggested it would be good to have a test covering the case these changes are fixing. To not break inadvertently it in the future.

ironirc commented 1 year ago

Hi, I just added the test case.

gcotelli commented 1 year ago

Thank you @ironirc