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

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

Merged

Conversation

ironirc
Copy link
Contributor

@ironirc ironirc commented Jun 28, 2023

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

@codecov
Copy link

codecov bot commented Jun 28, 2023

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%) ⬇️
src/Glorp/GlorpCursoredStream.class.st 75.35% <100.00%> (ø)

... and 35 files with indirect coverage changes

@astares
Copy link
Contributor

astares commented Jun 28, 2023

For the records: this is a substitute for #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
Copy link
Contributor Author

ironirc commented Jun 28, 2023

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 gcotelli self-requested a review June 29, 2023 13:11
@gcotelli
Copy link
Member

gcotelli commented Jun 29, 2023

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
Copy link
Contributor Author

ironirc commented Jul 23, 2023

Hi, I just added the test case.

@gcotelli gcotelli added the bug label Jul 24, 2023
@gcotelli gcotelli merged commit 851d7d3 into pharo-rdbms:master Jul 24, 2023
30 checks passed
@gcotelli
Copy link
Member

Thank you @ironirc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants