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

ensure the LEFT JOIN behaves as expected #623

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

bwdmr
Copy link
Contributor

@bwdmr bwdmr commented Jan 2, 2025

The WHERE clause will filter out these rows because NULL values do not satisfy either the IS NULL or the greater than conditions, effectively excluding them from the result set.

Turning the LEFT JOIN into an INNER JOIN for rows where NULL values would have been expected.

  • move includeDeleted checks from query to join, where the filter will be applied
  • copy filters over and add filter as necessary, on case deleted in not included
  • keep includeDeleted checks within query aswell, as null checks now can exists in where clause aswell.
  • adjust tests to reflect changes in code.

The WHERE clause will filter out these rows because NULL values do not satisfy either the IS NULL or the greater than conditions, effectively excluding them from the result set.

Turning the LEFT JOIN into an INNER JOIN for rows where NULL values would have been expected.

* move includeDeleted checks from query to join, where the filter will be applied
* copy filters over and add filter as necessary, on case deleted in not included
* keep includeDeleted checks within query aswell, as null checks now can exists in where clause aswell.
* adjust tests to reflect changes in code.
@bwdmr bwdmr requested a review from gwynne as a code owner January 2, 2025 13:02
Sources/FluentKit/Properties/Timestamp.swift Outdated Show resolved Hide resolved
Sources/FluentKit/Properties/Timestamp.swift Outdated Show resolved Hide resolved
Sources/FluentKit/Query/Builder/QueryBuilder+Join.swift Outdated Show resolved Hide resolved
Sources/FluentKit/Query/Builder/QueryBuilder.swift Outdated Show resolved Hide resolved
@bwdmr bwdmr force-pushed the leftjointimestamp branch from 7c4f2c7 to 88fbec7 Compare January 19, 2025 14:45
@bwdmr bwdmr requested a review from gwynne January 19, 2025 14:46
@bwdmr bwdmr force-pushed the leftjointimestamp branch from 33236f1 to 983cd12 Compare January 20, 2025 01:11
@bwdmr bwdmr requested a review from 0xTim January 23, 2025 05:01
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 24.48%. Comparing base (260eff9) to head (13e26ac).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
Sources/FluentKit/Model/AnyModel.swift 0.00% 1 Missing ⚠️
...es/FluentKit/Query/Builder/QueryBuilder+Join.swift 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
+ Coverage   24.28%   24.48%   +0.19%     
==========================================
  Files         149      149              
  Lines        9349     9375      +26     
==========================================
+ Hits         2270     2295      +25     
- Misses       7079     7080       +1     
Files with missing lines Coverage Δ
Sources/FluentKit/Properties/CompositeID.swift 88.46% <100.00%> (+0.46%) ⬆️
Sources/FluentKit/Properties/Timestamp.swift 87.37% <100.00%> (+2.49%) ⬆️
Sources/FluentKit/Query/Builder/QueryBuilder.swift 73.51% <100.00%> (ø)
Sources/FluentKit/Model/AnyModel.swift 42.30% <0.00%> (ø)
...es/FluentKit/Query/Builder/QueryBuilder+Join.swift 56.81% <88.88%> (+3.06%) ⬆️

@gwynne
Copy link
Member

gwynne commented Jan 31, 2025

It appears that the Mongo driver (or Mongo itself) can't handle this. @Joannis, can you provide any insight?


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excess whitespace change, please remove

@bwdmr
Copy link
Contributor Author

bwdmr commented Feb 7, 2025

support for complex joins with filters is not implemented yet.

https://github.com/vapor/fluent-mongo-driver/blob/79bf35430e72e4290b3d08ac1eb8784d4e389c32/Sources/FluentMongoDriver/MongoDB%2BJoin.swift#L114

joins with filter would require a pipeline as in the following example

db.orders.aggregate([
  { $lookup: {
    from: "customers",
    let: { 
      orderId: "$customerId",
      orderDate: "$orderDate" 
    },
    pipeline: [
      { $match: {
        $expr: {
          $and: [
            { $eq: ["$_id", "$$orderId"] },
            { $gt: ["$memberSince", "$$orderDate"] },
            { $eq: ["$isVerified", true] }
          ]
        }
      }}
    ],
    as: "customerInfo"
  }}
])

gwynne and others added 3 commits February 7, 2025 06:46
* Add missing targets to api-docs.yml

* Add missing targets to pathsToInvalidate
…or#628)

The CompositeIDProperty was force-unwrapping its value during database input,
which would crash if the value was nil. This could happen during initial
inserts where the composite ID is meant to be set by a database trigger.
Now checks if value exists before attempting to input it.
@gwynne
Copy link
Member

gwynne commented Feb 7, 2025

A solution for the Mongo driver is being worked on (though not by me, as I neither have nor want any knowledge of Mongo 😅)

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

Successfully merging this pull request may close these issues.

4 participants