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

All we need to do is listen. #99

Merged
merged 13 commits into from
Jul 8, 2024
Merged

All we need to do is listen. #99

merged 13 commits into from
Jul 8, 2024

Conversation

meksor
Copy link
Contributor

@meksor meksor commented Jul 5, 2024

Hi
I've replaced the current audit information setup (calling set_creation_info when creating an object) with a listener system in anticipation for the versioning change.

I've added the class SqlaEventHandler to be instantiated in each database backend. This handler listens to three events explained in the docstrings.

If the event targets inherit from the new mixins (by fridolin, moved and refactored by me here), special data will be injected into the queries that serves to set/update the relevant columns without an extra query dispatch. This is documented in the mixin docstrings.

The following test was added:

def test_audit_info(self, test_mp, request):
    test_mp = request.getfixturevalue(test_mp)
    run = test_mp.backend.runs.create("Model", "Scenario")
    test_mp.backend.runs.set_as_default_version(run.id)
    test_mp.backend.runs.create("Model", "Scenario")

    runs = test_mp.backend.runs.tabulate(default_only=False)
    assert (runs["created_by"] == "@unknown").all()
    # was updated by set_as_default_version
    assert runs["updated_by"][0] == "@unknown"
    assert runs["updated_by"][1] is None

As far as I can see this is the only place where the current rules apply. Run is the only model that can be updated directly (so i added HasUpdateInfo to its parents).

Note that this does not include the functionality to automatically update the UpdateInfo when related objects are changed.
For example when using Run.iamc.add or Run.iamc.remove.
This is on purpose as I think it will make these PRs easier to review. My plan is to implement some sort of "ownership" system that can be used for update information, orphan deletion and permission checking all at the same time. A short summary could be that we want to allow rows to own other rows along foreign keys. For example a Run owns multiple TimeSeries which in turn own multiple DataPoints. More to follow in a seperate PR.

More:

  • Removed async_client from api backend as it was emitting deprc. errors and is no longer used.

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks :)
Just one question about wording in-line :)

ixmp4/data/db/mixins.py Outdated Show resolved Hide resolved
@meksor meksor merged commit 9faa6a9 into main Jul 8, 2024
9 checks passed
@meksor meksor deleted the feature/udpate-audit-info branch July 8, 2024 09:27
@meksor meksor mentioned this pull request Jul 31, 2024
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.

2 participants