Skip DB execution when all ids for @OptionalParent are nil

[mtj0928]

OptionalParentEagerLoader doesn't need to fetch To objects when all ids of the given models are nil, because the results of the DB execution will always be empty.

Therefore, I've modified the logic to skip the DB execution in such cases to improve performance.

[gwynne]

This is already guaranteed by the QueryBuilder logic here: https://github.com/vapor/fluent-kit/blob/main/Sources/FluentKit/Query/Builder/QueryBuilder.swift#L234-L237

                // don't run eager loads if result set was empty
                guard !all.isEmpty else {
                    return self.database.eventLoop.makeSucceededFuture(())
                }

[mtj0928]

Your comment is correct for many propertyWrappers, but not for @OptionalParent. With OptionalParent, even if the provided models by the QueryBuilder have several elements, the set passed to the query can be empty.

[gwynne]

With OptionalParent, even if the provided models by the QueryBuilder have several elements, the set passed to the query can be empty.

Huh, so it can - I stand corrected!

However, even in this case we still need to proceed through the remainder of the method in order for the logic around the nilParentModels array to take effect (an admittedly confusing bit of code whose purpose is to ensure that the relation is treated as "loaded" with a nil value (as opposed to not loaded at all) for purposes of interoperating with Codable, not overwriting data during model updates, etc.

[mtj0928]

I set .some(nil) for all objects when the database execution is skipped. Isn't that enough?

https://github.com/mtj0928/fluent-kit/blob/021f7d59d970e524b4639f5844273cac1719455e/Sources/FluentKit/Properties/OptionalParent.swift#L178

[gwynne]

I humbly stand corrected a second time; for some reason I missed that line when I looked at your changes before 🤦‍♀️. My apologies for the confusion!

I've reopened this PR, and my only request for it is that you apply the same logic to the other optional relations:

• @OptionalChild (when referring to an @OptionalParent)
• @Children (when referring to an @OptionalParent)
• @CompositeOptionalParent
• @CompositeOptionalChild (when referring to a @CompositeOptionalParent)
• @CompositeChildren (when referring to a @CompositeOptionalParent)

[github-advanced-security[bot]]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[mtj0928]

I checked the mentioned propertyWrappers, but none of them seem to be able to predict the results of the database execution. Therefore, I believe the current changes are sufficient.

The current logic of @CompositeOptionalParent seems to be similar to that of @OptionalParent.

    func run(models: [From], on database: Database) -> EventLoopFuture<Void> {
        var sets = Dictionary(grouping: models, by: { $0[keyPath: self.relationKey].id })
        let nilParentModels = sets.removeValue(forKey: nil) ?? []


        let builder = To.query(on: database)
            .group(.or) { _ = sets.keys.reduce($0) { query, id in query.group(.and) { id!.input(to: QueryFilterInput(builder: $0)) } } }
        if (self.withDeleted) {
            builder.withDeleted()
        }
        return builder.all().flatMapThrowing {
                let parents = Dictionary(uniqueKeysWithValues: $0.map { ($0.id!, $0) })


                for (parentId, models) in sets {
                    guard let parent = parents[parentId!] else {
                        database.logger.debug(
                            "Missing parent model in eager-load lookup results.",
                            metadata: ["parent": "\(To.self)", "id": "\(parentId!)"]
                        )
                        throw FluentError.missingParentError(keyPath: self.relationKey, id: parentId!)
                    }
                    models.forEach { $0[keyPath: self.relationKey].value = .some(.some(parent)) }
                }
                nilParentModels.forEach { $0[keyPath: self.relationKey].value = .some(.none) }
            }
    }

https://github.com/vapor/fluent-kit/blob/ccea9820fe31076f994f7c1c1d584009cad6bdb2/Sources/FluentKit/Properties/CompositeOptionalParent.swift#L219C1-L243 However, the code fetches all To objects when the sets is empty. I'm unsure if the current logic is correct, so please let me know if it needs changes.

[mtj0928]

@gwynne Could you review this PR again?

[0xTim]

@gwynne ping

[gwynne]

Okay, this looks correct to me as it is now, and my apologies for the long delay in re-review! The only thing missing is a test of the behavior.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 47.33%. Comparing base (bb47433) to head (0b5b857).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #583      +/-   ##
==========================================
- Coverage   47.39%   47.33%   -0.07%     
==========================================
  Files         106      106              
  Lines        4756     4758       +2     
==========================================
- Hits         2254     2252       -2     
- Misses       2502     2506       +4     

Files	Coverage Δ
Sources/FluentKit/Properties/OptionalParent.swift	28.39% <0.00%> (-1.87%)	:arrow_down:

... and 1 file with indirect coverage changes

[mtj0928]

Thank you for reviewing the PR again.

I investigated the unit tests for FluentKit and found that it already includes tests for OptionalParent here: https://github.com/vapor/fluent-kit/blob/bb47433520116b3cf7f3567137c81106b398b77e/Sources/FluentBenchmark/Tests/OptionalParentTests.swift#L7-L89 .

The test covers both the nil case and the non-nil case, which I believe to be sufficient.

---

I understand the ideal approach is to verify that database execution is skipped, and I attempted to write a new test case to confirm that the DB execution is not run when the value is nil. However, it's a bit difficult to do so in the current design, because DummyDatabaseForTestSQLSerializer always returns a DummyRow, and we cannot control the row from outside.

I'm not familiar with this library, so please let me know if I miss something. Thnak you!

[gwynne]

Hm... yes, I believe that existing test suffices, thanks for looking into it!

[mtj0928]

@gwynne @0xTim The changes will help the performance of my app, thank you for responding to my PR!!