search

vapor / fluent-kit

Swift ORM (queries, models, and relations) for NoSQL and SQL databases
MIT License
207 stars 116 forks source link

Using Querybuilder with .field() or .fields() on models with optional relations crashes in SiblingsEagerLoader() #584

Open rchav opened 1 year ago

rchav commented 1 year ago

Describe the bug

See discussion in Discord for full context: https://discord.com/channels/431917998102675485/1146265856834281573/1146265856834281573

Seeing a crash when using .fields() or .field() on a QueryBuilder object that has optional relations (Siblings + OptionalParents in this case)

let queryBuilder = Planet.query(on: req.db)
    .with(\.$spacePrograms)
        .join(siblings: \.$spacePrograms) // 1.
    .with(\.$star)
        .join(parent: \.$star)
    .fields(for: SpaceProgram.self)
    .fields(for: Star.self)
    .unique()

// apply filters here...

// Crashes on this line
let matchedPlanets = try await queryBuilder.paginate(for: req)

Crash text:

FluentKit/Siblings.swift:372: Fatal error: Unexpectedly found nil while unwrapping an Optional value
2023-08-29 20:33:13.217344-0700 Run[29204:1632081] FluentKit/Siblings.swift:372: Fatal error: Unexpectedly found nil while unwrapping an Optional value

Expected behavior

Should not crash, return results for pagination.

Environment

.package(url: "https://github.com/vapor/vapor.git", exact: "4.65.0"),
.package(url: "https://github.com/vapor/fluent.git", exact: "4.7.0"),
.package(url: "https://github.com/vapor/fluent-postgres-driver.git", exact: "2.6.0"),

Additional context

cc: @gwynne

gwynne commented 1 year ago

This is technically a misuse of the API (excluding the main model's fields from the result means that its id is not available), but Fluent should not cause an inexplicable "unexpectedly found nil" fatal error in this case. Since it is API misuse, it should be a fatal error, but at the very least it should have a clear explanation and since the problem can be detected before the query actually runs, it definitely should be.

Additional notation: We could implicitly include the appropriate ID column regardless of the user's configuration, but doing so risks changing the functional meaning of a query, such as when using .unique() to deduplicate results.