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(141): handle after_flush_postexec when creating version objects #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

indiVar0508
Copy link
Contributor

when creating version objects if a person has created after_flush_postexec hook, which keeps calling after_flush untill it exhausts 100 attempts or session is no longer dirty, this is picked up by mapper as after_update which within same transaction adds a update operation type, so we have a check if target is already in UoW we continue with operation type that it is in untill transaction is completed.

@coveralls
Copy link

coveralls commented Aug 8, 2024

Pull Request Test Coverage Report for Build 10322605094

Details

  • 37 of 37 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 93.8%

Totals Coverage Status
Change from base Build 7974435756: 0.02%
Covered Lines: 4939
Relevant Lines: 5194

💛 - Coveralls

@indiVar0508 indiVar0508 force-pushed the fix_141 branch 2 times, most recently from c58761e to 2cc9de3 Compare August 9, 2024 05:28
article2.id = article.id
# we were earlier updating ID which didn't seem right, so changed this to name since
# id is used by us for identity in operation
article2.name = article.name
Copy link
Contributor

@AbdealiLoKo AbdealiLoKo Aug 9, 2024

Choose a reason for hiding this comment

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

Actually updating ID seems to be the reason this testcase was written

If you see the skipif - it is trying to update the "identity" and seeing how that works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that skipif was added by us only, when we were extending package support to mssql db, i changed it because changing ID won't be right in real sense and i tried it out as well
.
say i change the ID of article2 to article now if i try to collect version objects of article2 i'll get version object of article1
.
i missed removing the skipif, i'll remove the skipif since we no longer update the identity so this should not fail with mssql.

@indiVar0508 indiVar0508 force-pushed the fix_141 branch 2 times, most recently from 2c303d2 to cdb8abc Compare August 9, 2024 16:34
when creating version objects if a user has created after_flush_postexec
hook, which keeps calling `after_flush` untill it exhausts 100 attempts
or session is no longer dirty, this is picked up by mapper as after_update
which within same transaction adds a update operation type, so we have a
check if target is already in UoW we continue with operation type that
it is in untill transaction is completed.

We also updated a existing testcase named `test_replace_deleted_object_with_update`
as it was updating the pk of article object, but the pk being identity of object
is used by operations to track target, so changing a non identity column to validate
partial flush does not impact other objects
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.

Sqla-history not able to handle after_flush_postexec hook
3 participants