Skip to content

Add support for nested sessions (SAVEPOINTs) in modifications tracking#358

Closed
barakalon wants to merge 2 commits intopallets-eco:mainfrom
barakalon:track-changes-to-support-nesting
Closed

Add support for nested sessions (SAVEPOINTs) in modifications tracking#358
barakalon wants to merge 2 commits intopallets-eco:mainfrom
barakalon:track-changes-to-support-nesting

Conversation

@barakalon
Copy link

I love the signals for tracking modifications to models. However, they don't yet support nested sessions.

Here is a proposal for adding support for this. It uses a stack of dictionaries instead of just a dictionary for tracking models that have been inserted/updated/deleted. When a nested session begins, a new dictionary is pushed to the stack. If that nested session is rolled back, the changes that happened during that session are removed. If that nested session is committed, the changes are merged into the next item.

Anywho, let me know what you think! This is my first attempt at a contribution to flask-sqlalchemy, so take it easy on me 😁

@barakalon barakalon force-pushed the track-changes-to-support-nesting branch 2 times, most recently from 7173c8d to 99f6308 Compare December 5, 2015 02:11
@davidism
Copy link
Member

Thanks, this looks interesting. I'll try to take a look at it soon, just commenting to let you know I'm aware of it. :-)

@davidism davidism self-assigned this Dec 11, 2015
@barakalon
Copy link
Author

Thanks @davidism

I've been using this in production for about a month now and it's working fine.

@immunda
Copy link
Contributor

immunda commented Jan 8, 2016

Whilst this change in the current context looks good, there are plans to deprecate Flask-SQLAlchemy specific signals (#275), as SQLAlchemy signals have come a long way since this library was created and some of the signals are quite flawed. This is also inline with plans to not track modifications by default as it adversely affects performance.

Happy to have input on these decisions, especially @davidism as he's input on this thread.

@barakalon
Copy link
Author

@immunda Just to make sure I'm on the same page, you mean SQLAlchemy's events, yes?

@immunda
Copy link
Contributor

immunda commented Jan 10, 2016

Sorry, yes, events. Some Django parlance is hard to shake.

I also think you should actually ignore my previous comment regarding #275, since #364 now fixes problems that led to that decision.

@barakalon barakalon force-pushed the track-changes-to-support-nesting branch from 99f6308 to 76c300c Compare January 10, 2016 18:16
@barakalon
Copy link
Author

Ah, right on.

Well when tracking modifications is disabled by default, support for savepoints will still be a good thing.

@barakalon barakalon closed this Aug 29, 2016
@davidism
Copy link
Member

Why did you close this?

@barakalon
Copy link
Author

Ah, just because its been open for many months and I was doing a bit of house cleaning. I figured it was dead.

@davidism davidism reopened this Aug 29, 2016
@roganov
Copy link

roganov commented Sep 12, 2016

Would be interesting to know how much this affects performance since per @immunda even the current naive implementation affects performance "adversely".

@@ -178,67 +178,86 @@ class _SessionSignalEvents(object):
@classmethod
def register(cls, session):
if not hasattr(session, '_model_changes'):
Copy link

Choose a reason for hiding this comment

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

should be _model_changes_stack here?


@classmethod
def unregister(cls, session):
if hasattr(session, '_model_changes'):
Copy link

Choose a reason for hiding this comment

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

ditto

@davidism davidism removed their assignment Oct 4, 2017
@rsyring rsyring added this to the Unknown milestone Mar 9, 2019
@davidism
Copy link
Member

#1087 changes modification tracking, so this will no longer be mergeable. I also have the impression that how SQLAlchemy tracks nested transactions has changed, but I'm not sure. That said, after considering it more, I don't want to add more complexity to the track modifications feature. If you need other behavior, you're better off using SQLAlchemy's events directly.

@davidism davidism closed this Sep 18, 2022
@davidism davidism removed this from the Unsure milestone Sep 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants