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
Modifications to next hid_counter generation #12948
Conversation
history_id = cached_id(self) | ||
update_stmt = update(table).where(table.c.id == history_id).values(hid_counter=table.c.hid_counter + n) | ||
|
||
with engine.begin() as conn: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That creates a new commit, which we don't want. Why is this needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
item 2 in the PR description. In my understanding, there's no autocommit in SA2.0. i.e., it would work now using with endine.connect() as conn
, but unless we did conn.commit()
in the end, it wouldn't work as soon as we moved to SA2.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you verify we're not expiring all attributes here ? If we are this would be a huge performance penalty. It seems we might not be, since you're explicitly expiring hid_counter ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test for that very reason 😃 4ea1185
h._next_hid() | ||
|
||
assert 'hid_counter' in state.unloaded # this attribute has been expired | ||
assert 'id' not in state.unloaded # but other attributes have NOT been expired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so because we bypass the current session completely this works. That's neat!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if it didn't it would've probably wreaked havoc in the tests, where a lot of HDAs would be committed with a hid, which would raise exceptions now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I also turned on echo=True for the engine when I was working on this to make sure there were no surprises.
Thank you! |
A few modifications triggered by migration to SQLAlchemy 2.0 (ref #12651
Solution: use an explicit connection.
Solution: use SQLAlchemy Core to obtain a connection from the engine: this will limit the commit to the operation.
Note: this potentially affects current behavior whenever we access a history's hid_counter: (a) in the
ModelImportStore
, (b) in thedev-detailed
andbetawebclient
views and the_serialize
History method, (c) in the legacy history controller, and (d) in thecopy
History method. The effects would be (1) a call to the database to refresh the attribute, and (2) the value may be different (but correct). But our tests pass.Ping @assuntad23, @dannon: although (I think) this is unlikely, this might affect the client (iff there was a case when the supplied value of hid_counter was incorrect - i.e., outdated due to calls to db_next_hid)
How to test the changes?
(Select all options that apply)
License