-
Notifications
You must be signed in to change notification settings - Fork 6
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
Implement safeguard against problematic meta keys #131
Implement safeguard against problematic meta keys #131
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
=====================================
Coverage 85.8% 85.9%
=====================================
Files 227 227
Lines 7564 7583 +19
=====================================
+ Hits 6494 6514 +20
+ Misses 1070 1069 -1
|
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.
If I am not mistaken, a check in RunMetaEntryRepository.bulk_upsert
is missing!
tests/data/test_meta.py
Outdated
def test_illegal_key(self, platform: ixmp4.Platform): | ||
run = platform.runs.create("Model", "Scenario") | ||
with pytest.raises(InvalidRunMeta): | ||
platform.backend.meta.create(run.id, "version", "foo") | ||
|
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.
def test_illegal_key(self, platform: ixmp4.Platform): | |
run = platform.runs.create("Model", "Scenario") | |
with pytest.raises(InvalidRunMeta): | |
platform.backend.meta.create(run.id, "version", "foo") | |
def test_illegal_key(self, platform: ixmp4.Platform): | |
run = platform.runs.create("Model", "Scenario") | |
for illegal_key in {"model", "scenario", "id", "version", "is_default"}: | |
with pytest.raises(InvalidRunMeta): | |
platform.backend.meta.create(run.id, illegal_key, "foo") |
Might be good to test all the keys from a distinct set here so we have to think about what we broke when removing keys from the actual set.
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.
Added a loop for testing all illegal keys.
Also not quite sure what exactly the problem is, i think the api currently does not allow for filtering my meta indicators... So is this check here now to account for something going on in the frontend implementation of the data explorer? |
Thanks @meksor, added the check also to For clarification, the aim of this PR is to prevent users from setting meta indicators that could be confusing, irrespective of whether it's confusing for the front-end API or for other users. |
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 see
Just ran into an issue (with @pmussak) in the new ECEMF app and ixmp4 database instance - I had added meta-indicators named "version" to the database, which was "overwriting" the version-attribute in the run-selection component of the app.
To avoid such issues, this PR makes several terms illegal as meta-indicator-keys (at creation). I briefly thought about adding this to the meta-model (see link below), but I was worried that if a meta indicator with such a name was already created in a database with a current ixmp4 version, there might be issues with then loading them after upgrading ixmp4.
ixmp4/ixmp4/data/db/meta/model.py
Line 64 in 70366d4