populationgenomics / metamist

Sample level metadata system
MIT License
1 stars 1 forks source link

Forbid fields other than those defined by each model class #752

Closed jmarshall closed 4 months ago

jmarshall commented 4 months ago

This PR activates field strictness, as identified in #746. This may or may not be a good idea, but it identifies a few existing problems hence so far I like it… :smile:

This would have detected the sample_type/sample_types problem (https://github.com/populationgenomics/metamist/pull/615#issuecomment-2071594382) that took a bit of debugging at the end of custom-cohorts.

It identifies where we've left a real-world field that exists in the database out of SequencingGroupInternal though apparently we don't use it at present (as the omission went unnoticed)[Resolved a different way — see below]. It also identifies that ProjectSummary.links was probably never being set.

The second commit's fixes are enough to make the tests pass again. There may possibly be other models needing adjustments that have been missed because they are not exercised by tests…

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.15%. Comparing base (091b26d) to head (1bd3149).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #752 +/- ## ======================================= Coverage 77.15% 77.15% ======================================= Files 157 157 Lines 12945 12946 +1 ======================================= + Hits 9988 9989 +1 Misses 2957 2957 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jmarshall commented 4 months ago

I updated _db/python/tables/sequencinggroup.py so that sg.author is omitted from SequencingGroupInternal et al. I checked the three places where common_get_keys_str is used:

Hence removing it should be fine.

jmarshall commented 4 months ago

Thanks for reviewing. I'll merge it and start taking advantage of it. And hopefully nothing breaks 🤞

jmarshall commented 4 months ago

I forgot to mention one possible issue flying just below the radar: some of the Family and Analysis models (and some model classes over in api/routes/*.py) inherit from pydantic's BaseModel instead of our SMBase, so they won't get this SMBase.Config setting to set forbid mode.

I'm happy to have just hit most of the model classes here, but… what's up with those classes inheriting directly from BaseModel?