Skip to content

new signal: post_bulk_update #524

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

Merged
merged 12 commits into from
Jan 14, 2022
Merged

new signal: post_bulk_update #524

merged 12 commits into from
Jan 14, 2022

Conversation

ponytailer
Copy link
Contributor

@ponytailer ponytailer commented Jan 14, 2022

  • fix typo
  • add new signal after bulk_update

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

Merging #524 (3373bbc) into master (1ffb28d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #524   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          185       185           
  Lines        15232     15253   +21     
=========================================
+ Hits         15232     15253   +21     
Impacted Files Coverage Δ
ormar/decorators/__init__.py 100.00% <ø> (ø)
ormar/decorators/signals.py 100.00% <100.00%> (ø)
ormar/exceptions.py 100.00% <100.00%> (ø)
ormar/models/metaclass.py 100.00% <100.00%> (ø)
ormar/models/mixins/save_mixin.py 100.00% <100.00%> (ø)
ormar/queryset/queryset.py 100.00% <100.00%> (ø)
tests/test_queries/test_queryset_level_methods.py 100.00% <100.00%> (ø)
tests/test_signals/test_signals.py 100.00% <100.00%> (ø)

@ponytailer
Copy link
Contributor Author

ponytailer commented Jan 14, 2022

Sorry, I saw the part of changes including #520 just now

And maybe add the new signal called post_bulk_create and pre_bulk_create ?
better than List[post_create] @collerek

@collerek
Copy link
Owner

Yes, I agree that if implemented those should be separate signals that accept a list of instances -> that way you can skip the bool flag from method signatures and still have some signals only for "normal" create or update but not for bulk operations.

But there is a bigger problem and a reason why this is not present.
I intentionally didn't add signals to bulk operations because bulk operations in any of the supported drivers do not return the updated data. So marking those objects as saved in your code is not really true, as you won't have primary key values for autoincrement fields, or won't have updated columns that have defaults set in the database.

And since they don't have pks (or at least don't have to have them) there is no reliable way to refresh the models after bulk_create, so the signal in your code receives incomplete data (each instance in db differs from the one you have in bulk operation).

@ponytailer
Copy link
Contributor Author

ponytailer commented Jan 14, 2022

Yes, I agree that if implemented those should be separate signals that accept a list of instances -> that way you can skip the bool flag from method signatures and still have some signals only for "normal" create or update but not for bulk operations.

But there is a bigger problem and a reason why this is not present. I intentionally didn't add signals to bulk operations because bulk operations in any of the supported drivers do not return the updated data. So marking those objects as saved in your code is not really true, as you won't have primary key values for autoincrement fields, or won't have updated columns that have defaults set in the database.

And since they don't have pks (or at least don't have to have them) there is no reliable way to refresh the models after bulk_create, so the signal in your code receives incomplete data (each instance in db differs from the one you have in bulk operation).

Got it. So I add the new signal called post_bulk_update to complete it.

And maybe need the flush to get the primary key before execute sql when bulk_create

@ponytailer ponytailer changed the title add post-signal in bulk-update & bulk-create new signal: post_bulk_update Jan 14, 2022
@ponytailer
Copy link
Contributor Author

Thanks for your review. Please release when it was merged.

I need this signal to develop project.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@collerek
Copy link
Owner

Awesome, thanks!

@collerek collerek merged commit 3260106 into collerek:master Jan 14, 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.

None yet

3 participants