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

feat: implement Model._UNSTABLE_destroyMany #17031

Merged
merged 20 commits into from Feb 10, 2024
Merged

feat: implement Model._UNSTABLE_destroyMany #17031

merged 20 commits into from Feb 10, 2024

Conversation

ephys
Copy link
Member

@ephys ephys commented Feb 1, 2024

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

This PR implements #15459 for non-paranoid deletes, making it possible to delete multiple instances in one query:

// create 2 users in one query
const users = await User.bulkCreate([{}, {}]);

// delete 2 users in one query
await User.destroyMany(users);

Following PRs will implement

  • soft deleting & making modelInstance.destroy call Repository.destroy
  • saveMany
  • updateMany
  • restoreMany
  • Documenting all of them

Technical Notes

This PR also introduces a number of new practices:

  • Add support for Repositories / Data Mapper #15389: This method is the first method to use the repository approach. The model delegates to the repository. As we slowly migrate everything to the repository, we'll one day be able to have a proper repository system that allows using the same entities in multiple databases.
  • omitHook #14053: This method supports the new hook option, which allows specifying exactly which hook will run instead of a global enable/disable. Because hooks are always enabled by default, the new hook option is called noHooks as it's clearer for booleans to be false by default.
  • The option to hard delete paranoid instances is called hardDelete. force has been deprecated in favor of hardDelete, as what it does is clearer than force (considering a paranoid delete is a soft delete)
  • Added a new type TableOrModel, that aims to replace TableNameOrModel: This type supports repositories as well
  • This method uses a new approach for hook value mutability: instead of deep cloning the value, which is expensive, and having to handle two different typings for them (as the input can be immutable, but the value sent to the hook must be mutable):
    • The new approach shallow clones the options object, and makes it mutable, but only at the top level. All nested values are immutable. This immutability is only applied in development environments to avoid the overhead in production. This should greatly improve the performances, and means we can make our types readonly.
  • Finally, considering how many changes were made just for the basic destroy method, I think the approach I'm going to take for the migration of model is to move the logic into model-repository method by method, but keep the old code as-in in model.js, and in v8 make model.js call model-repository.

@ephys
Copy link
Member Author

ephys commented Feb 1, 2024

note to myself for when i have more time to work on this: the repository should be able to receive breaking changes during V7, so the getter on models should be prefixed with unstable to reflect this

@ephys
Copy link
Member Author

ephys commented Feb 2, 2024

I included a fix for the deprecation notice discovered in #15934 (comment). Considering that the method changed already, I opted to removed delete altogether as there is no point in making users update to an already deprecated method

@WikiRik
Copy link
Member

WikiRik commented Feb 2, 2024

Just as a sidenote; we can just remove TableNameOrModel in a next PR. We introduced it in the alphas, so no breaking change compared to v6. I did a quick replace and added additional tests in the query generator unit tests and only one is failing (bulkDeleteQuery with a limit set, since it will check for isModelStatic). See the commit here; 282efc1

@ephys
Copy link
Member Author

ephys commented Feb 5, 2024

More thoughts on this: I started working on the paranoid code path for this function but I wanted to make it use bulkUpdate, so I started migrating bulkUpdate to the repository code, while updating it to the new API. The problem is that it would send something incompatible to the hook, so I pretty much have to add a new _UNSTABLE_bulkUpdate hook too.

So I think I should do the following in this PR:

  • Rename Model.destroyMany to Model._UNSTABLE_destroyMany
  • Remove the new deprecations
  • Remove the _UNSTABLE_ prefix from the repository
  • Rename the repository functions to prefix them with _UNSTABLE_

And I think we should continue to slowly migrate model methods to the repository, implementing them using the new standards, then in v8 make them the default methods

@ephys ephys requested a review from WikiRik February 8, 2024 12:26
@ephys
Copy link
Member Author

ephys commented Feb 8, 2024

I think this is ready to be merged. I want to do the update one next once #17063 is merged

@ephys ephys changed the title feat: implement Model.destroyMany feat: implement Model._UNSTABLE_destroyMany Feb 10, 2024
@ephys ephys enabled auto-merge February 10, 2024 14:08
@ephys ephys added this pull request to the merge queue Feb 10, 2024
Merged via the queue into main with commit 1fead8a Feb 10, 2024
52 checks passed
@ephys ephys deleted the ephys/delete-many branch February 10, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To document
Development

Successfully merging this pull request may close these issues.

None yet

2 participants