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

fix: detach not invoke delete lifecycle #534

Closed
wants to merge 1 commit into from

Conversation

AFutureD
Copy link

This PR is aiming to fix the issue #533 and vapor/fluent#747.

The SiblingsProperty.detach(_:on:) miss to invoke delete lifecycle, while SiblingsProperty.attach(_:on:) does.

The last line .delete() just simply build an sql and runs it, which cause some issues.

    public func detach(_ tos: [To], on database: Database) -> EventLoopFuture<Void> {
        guard let fromID = self.idValue else {...}
        let toIDs = tos.map {...}

        return Through.query(on: database)
            .filter(self.from.appending(path: \.$id) == fromID)
            .filter(self.to.appending(path: \.$id) ~~ toIDs)
            .delete()
    }

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're going to have to be a bit cleverer here and manually invoke the lifecycle or similar because at the moment we can't accept the patch as is as it turns a functions that performs one SQL query into a function that performs as many queries as there are siblings which would be a big performance hit

@0xTim
Copy link
Member

0xTim commented Sep 29, 2022

cc @gwynne

@AFutureD
Copy link
Author

Agreed. This PR does have some potential performance issues. But, it might be a little difficult to figure out the cleaver solution. Waiting for your idea.

@AFutureD
Copy link
Author

AFutureD commented Sep 29, 2022

If we could get all models first, and then invoke lifecycle manually.
After that, we delete as before.

@0xTim Is it an option?

Pseudocode may as follows:

let query = Through.query(on: database)
            .filter(self.from.appending(path: \.$id) == fromID)
            .filter(self.to.appending(path: \.$id) ~~ toIDs)

try await query.all().map { $0.invoke() }

try await query.delete()

@0xTim
Copy link
Member

0xTim commented Sep 30, 2022

Unfortunately that still loads potentially a very large number of pivots in to memory which can be slow. I'm going to defer to @gwynne when she gets a chance to look at it as it's her codebase and she's the expert on all things Fluent

@AFutureD AFutureD closed this Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants