Skip to content

Conversation

@gwynne
Copy link
Member

@gwynne gwynne commented Mar 11, 2025

These changes are now available in 1.50.4

As with @CompositeParent, keypaths through @CompositeOptionalParent can refer to properties of the target model's composite IDValue. However, the subscript(dynamicMember:) accessor of @CompositeOptionalParent which enables this functionality returns an optional value, and the result is always nil if the relation has not been loaded.

This was caused by an oversight during the original implementation of the composite relation properties: The accessor should not be returning an optional value, nor should the result ever be nil regardless of whether or not the relation is loaded. We can't fix the fact that it returns an optional without breaking public API, so callers still have to add a ! in the nested keypaths. But we can at least make the accessor useful by fixing it to never return nil, which is what this PR does. A test is also included.

Also updates the CI while we're at it.

Example (omits some boilerplate for brevity):

final class ParentModel: Model, @unchecked Sendable {
    final class IDValue: Fields, @unchecked Sendable {
        @Field(key: "foo")
        var foo: String
    }

    @CompositeID var id: IDValue?
}

final class ChildModel: Model, @unchecked Sendable {
    @ID
    var id: UUID?

    @CompositeOptionalParent(prefix: "parent")
    var parent: ParentModel?
}

// This would previously print `nil`.
print(ChildModel()[keyPath: \ChildModel.$parent.$foo?.queryablePath][0]) // prints Optional("foo")

…ferenced model's IDValue are never actually nil.
@gwynne gwynne added bug Something isn't working semver-patch Internal changes only labels Mar 11, 2025
@gwynne gwynne requested review from 0xTim, MahdiBM and ptoffy March 11, 2025 23:13
@codecov
Copy link

codecov bot commented Mar 11, 2025

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 24.26%. Comparing base (4f886eb) to head (dfb9f50).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Sources/FluentBenchmark/Tests/SchemaTests.swift 0.00% 2 Missing ⚠️
...ources/FluentBenchmark/Tests/SoftDeleteTests.swift 0.00% 1 Missing ⚠️
Sources/FluentBenchmark/Tests/UniqueTests.swift 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #632      +/-   ##
==========================================
+ Coverage   24.22%   24.26%   +0.03%     
==========================================
  Files         149      149              
  Lines        9348     9348              
==========================================
+ Hits         2265     2268       +3     
+ Misses       7083     7080       -3     
Files with missing lines Coverage Δ
...FluentKit/Properties/CompositeOptionalParent.swift 50.60% <100.00%> (+2.40%) ⬆️
...ources/FluentBenchmark/Tests/SoftDeleteTests.swift 0.00% <0.00%> (ø)
Sources/FluentBenchmark/Tests/UniqueTests.swift 0.00% <0.00%> (ø)
Sources/FluentBenchmark/Tests/SchemaTests.swift 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gwynne gwynne changed the title Partial fix for nested keypaths going through @CompositeOptionalParent properties Partial fix for nested keypaths through @CompositeOptionalParent properties Mar 11, 2025
@gwynne gwynne merged commit f12a668 into main Mar 12, 2025
15 of 16 checks passed
@gwynne gwynne deleted the fix-compositeoptionalparent-nested-properties branch March 12, 2025 00:14
gwynne added a commit that referenced this pull request Apr 11, 2025
…operties (#632)

* Fix @CompositeOptionalParent so that keypaths to properties of the referenced model's IDValue are never actually nil.

* Update CI versions for Swift and databases, enable Musl and Android tests

* Apply updated theme settings to DocC catalogs, update Swift version in README

* Fix a couple of warnings in tests, use a non-libc-dependent version of sleep() in a test

* Disable the fields property performance test (breaks Android tests and isn't very useful)
bwdmr pushed a commit to bwdmr/fluent-kit that referenced this pull request Dec 17, 2025
…operties (vapor#632)

* Fix @CompositeOptionalParent so that keypaths to properties of the referenced model's IDValue are never actually nil.

* Update CI versions for Swift and databases, enable Musl and Android tests

* Apply updated theme settings to DocC catalogs, update Swift version in README

* Fix a couple of warnings in tests, use a non-libc-dependent version of sleep() in a test

* Disable the fields property performance test (breaks Android tests and isn't very useful)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working semver-patch Internal changes only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants