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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -621,7 +621,7 @@ def get_latest_entry(entries): | |
self.new_hda(h1, name="1") | ||
self.new_hda(h2, name="2") | ||
self.session().flush() | ||
# db_next_hid modifies history, plus trigger on HDA means 2 additional audit rows per history | ||
# _next_hid modifies history, plus trigger on HDA means 2 additional audit rows per history | ||
|
||
h1_audits = get_audit_table_entries(h1) | ||
h2_audits = get_audit_table_entries(h2) | ||
|
@@ -920,6 +920,31 @@ def test_can_manage_private_dataset(self): | |
assert security_agent.can_manage_dataset(u_from.all_roles(), d1.dataset) | ||
assert not security_agent.can_manage_dataset(u_other.all_roles(), d1.dataset) | ||
|
||
def test_history_hid_counter_is_expired_after_next_hid_call(self): | ||
u = model.User(email="hid_abuser@example.com", password="password") | ||
h = model.History(name="History for hid testing", user=u) | ||
self.persist(u, h) | ||
state = inspect(h) | ||
assert h.hid_counter == 1 | ||
assert 'hid_counter' not in state.unloaded | ||
assert 'id' not in state.unloaded | ||
|
||
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 commentThe 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 commentThe 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 commentThe 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. |
||
assert h.hid_counter == 2 # check this last: this causes thie hid_counter to be reloaded | ||
|
||
def test_next_hid(self): | ||
u = model.User(email="hid_abuser@example.com", password="password") | ||
h = model.History(name="History for hid testing", user=u) | ||
self.persist(u, h) | ||
assert h.hid_counter == 1 | ||
h._next_hid() | ||
assert h.hid_counter == 2 | ||
h._next_hid(n=3) | ||
assert h.hid_counter == 5 | ||
|
||
def _three_users(self, suffix): | ||
email_from = f"user_{suffix}e1@example.com" | ||
email_to = f"user_{suffix}e2@example.com" | ||
|
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 didconn.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