❓ does fetchOne include all relationships by default? I recall seeing the relationships being fetched separately in toPOSProduct(db:). From the documentation, including seems to be for including the relationships which aren't fetched by default.

No, it doesn't.

You're right, including lets us get more than one thing back at a time, via the relationships. However, we need dedicated wrapper record types to use it, I think.

For example, we could:

        struct DetailedVariation: Decodable, FetchableRecord {
            var variation: PersistedProductVariation
            var image: PersistedProductVariationImage
            var attributes: [PersistedProductVariationAttribute]
        }

        let fetchedDetailedVariation = try db.read { db in
            try PersistedProductVariation
                .including(optional: PersistedProductVariation.image)
                .including(all: PersistedProductVariation.attributes)
                .asRequest(of: DetailedVariation.self)
                .fetchAll(db)
        }

Perhaps those wrappers would be worthwhile, even though they're another small layer. WDYT? We could use them in the tests to avoid relying on our POSProduct extensions.

With that approach, we need to do things like #expect(fetchedDetailedVariation.variation.id == 500), which isn't so nice. I guess we could use computed vars to get the base properties on the root of the detailed model, but it would be best if we could auto-generate those.

I've done an example in the test for now – see what you think, as it might be more convenient to fetch things this way before converting to POS models. See a9048a2

This seems related to my question in another PR about why relationships are fetched separately per product/variation #16063 (comment). Now I'm connecting the dots, that our Persisted*'s relationships aren't properties but associated types, which is a difference from Core Data managed objects.

Perhaps those wrappers would be worthwhile, even though they're another small layer. WDYT?

Yea, I think the wrappers that include the relationships for the use case are nice. Could be nice outside of unit tests in our POS use cases as well.

My question about toPOSProduct(db:) in the linked thread in another PR was mainly for the POS use case, do we plan to:

• Fetch all products/variations including relationships in one db request like in the example code above a9048a2. Then map the list of wrapper objects to POS product/variation list.
• Fetch all products/variations without relationships, then in the same db read transaction, map each Persisted* in the list with toPOSProduct(db:) to fetch relationships for each product/variation then convert to POS product/variation.

Not sure if there's significant performance difference, the first approach feels a bit more intuitive and efficient to me, but either way is probably fine since we fetch them in a paginated way in POS like 20 at a time.

Btw I tested and it's possible to keep the properties in the FetchableRecord wrapper constant, even though the doc used variables as well:

struct DetailedVariation: Decodable, FetchableRecord {
    let variation: PersistedProductVariation
    let image: PersistedProductVariationImage
    let attributes: [PersistedProductVariationAttribute]
    
    ...
}

Btw I tested and it's possible to keep the properties in the FetchableRecord wrapper constant, even though the doc used variables as well

Yeah, that makes sense, I wasn't looking too closely at that, just seeing how it worked. The thing I don't like about it is having to dig around in detailedVariation.variation for stuff, nor having to keep computed vars matching the base variation model. But there we are – we can do it by hand and invest time in automation when it's worthwhile.

Yea, I think the wrappers that include the relationships for the use case are nice. Could be nice outside of unit tests in our POS use cases as well.

Yeah, the unit tests just seemed a good place to try it out. If we want to keep it, they can be promoted to Yosemite.

My question about toPOSProduct(db:) in the linked thread in another PR was mainly for the POS use case, do we plan to:

Fetch all products/variations including relationships in one db request like in the example code above a9048a2. Then map the list of wrapper objects to POS product/variation list.
Fetch all products/variations without relationships, then in the same db read transaction, map each Persisted* in the list with toPOSProduct(db:) to fetch relationships for each product/variation then convert to POS product/variation.

Yes, it'll be paged so it probably doesn't matter too much. If that turns out to be true, I think it's probably best to fetch everything in one request, with relationships, and make them into POS models from the wrapper. We may want multiple ways to read from the db for other use cases, but that'll be the main one for display.

nit: could be let?

No, because of the auto incremented ID making insert mutating

super nit: can add Given/When/Then for consistency (same for test_product_without_related_records_creates_empty_arrays).

super nit: similar to above, can add Given/When/Then for consistency (same for test_product_variation_without_image_returns_nil).