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

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

Open
rchav opened this issue Aug 30, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@rchav
Copy link

rchav commented Aug 30, 2023

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"),
  • OS version: macOS 13.5.1

Additional context

cc: @gwynne

@rchav rchav added the bug Something isn't working label Aug 30, 2023
@gwynne
Copy link
Member

gwynne commented Aug 30, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants