Chore: 270 schema models #1883
Chore: 270 schema models #1883
Conversation
dleard
force-pushed
the
chore/270-schema-models
branch
from
0b27f4b to
c969e2e
Compare
| from django.db import models | ||
|
|
||
|
|
||
| class JsonSchema(BaseModel): |
There was a problem hiding this comment.
Let's make individual tables by object. This single table could become a pain to navigate
dleard
force-pushed
the
chore/270-schema-models
branch
5 times, most recently
from
d0476ea to
3b5004f
Compare
dleard
force-pushed
the
chore/270-schema-models
branch
2 times, most recently
from
e529e42 to
bfc71f9
Compare
| ActivitySchema.objects.create( | ||
| reporting_activity_id=ReportingActivity.objects.get(name='General stationary combustion').id, | ||
| json_schema=schema, | ||
| valid_from_id=Configuration.objects.get(valid_from='2024-01-01').id, |
There was a problem hiding this comment.
I'm wondering why do we use the apps.get_model for ActivitySchema vs the straight up import of the model module for Configuration here. Does it matter for migrations?
There was a problem hiding this comment.
@Sepehr-Sobhani Do you have any thoughts on this?
There was a problem hiding this comment.
I found reasons @dleard @Sepehr-Sobhani :
the import ReportingActivity here would import the model code from django. There is a chance, because we are running this in a migration, that some changes to that model are not deployed to the database yet and are part of a later migration to be run next.
Using the apps.get_model("reporting", "ReportingActivity") guarantees that we get the model object as it's been deployed to the database
There was a problem hiding this comment.
Exactly ⬆️ . Also, Using get_model in Django migrations ensures model decoupling, maintains forward and backward compatibility, and avoids import errors by dynamically loading the model state from the migration creation time.
https://codereview.doctor/features/django/best-practice/avoid-model-imports-in-migrations
| cwd = os.getcwd() | ||
| with open(f'{cwd}/reporting/json_schemas/2024/general_stationary_combustion/with_useful_energy.json') as gsc_st1: | ||
| schema1 = json.load(gsc_st1) | ||
| with open(f'{cwd}/reporting/json_schemas/2024/general_stationary_combustion/with_useful_energy.json') as gsc_st2: |
There was a problem hiding this comment.
| with open(f'{cwd}/reporting/json_schemas/2024/general_stationary_combustion/with_useful_energy.json') as gsc_st2: | |
| with open(f'{cwd}/reporting/json_schemas/2024/general_stationary_combustion/without_useful_energy.json') as gsc_st2: |
| Override the save method to validate if there are overlapping records. | ||
| """ | ||
| exception_message = f'This record will result in duplicate json schemas being returned for the date range {self.valid_from.valid_from} - {self.valid_to.valid_to} as it overlaps with a current record or records' | ||
| from reporting.utils import validate_overlapping_records |
There was a problem hiding this comment.
PEP8 recommends that imports are at the top of the file:
Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.
There was a problem hiding this comment.
Putting the import at the top of the file creates a circular dependency as we import ActivityJsonSchema in order to type the object being passed to utils function being called just below this line
There was a problem hiding this comment.
I think this is a sign that our typing needs work.
We can probably make a base model type with the valid_to and valid_from fields, and have our model classes extend that.
The validate_overlapping_records function would then accept a parameter of that base model type
There was a problem hiding this comment.
something like:
T = TypeVar('T', bound=ValidToFromBaseModel)
def validate_overlapping_records(object_class: T, ...):
...There was a problem hiding this comment.
There was a problem hiding this comment.
I'd like to move on from this ticket as it's a significant blocker for a lot of other work. I've set the types for the parameters to Any and created a tech debt ticket to apply more specific typing
bcgov/cas-reporting#276
There was a problem hiding this comment.
I suggest bending PEP8 recommendation and using inline import here, rather than using the Any type, as it completely ignores the benefits of using mypy.
There was a problem hiding this comment.
Yep! Let's fix this at another time / not at all.
Also agreed with Sepehr, maybe we don't have to treat Python as a statically typed language all the time
There was a problem hiding this comment.
The function is tested in each model's test that uses it. So there is some confidence there that it is working as expected
| Override the save method to validate if there are overlapping records. | ||
| """ | ||
| exception_message = f'This record will result in duplicate json schemas being returned for the date range {self.valid_from.valid_from} - {self.valid_to.valid_to} as it overlaps with a current record or records' | ||
| from reporting.utils import validate_overlapping_records |
| f'This record will result in duplicate configurations being returned for the date range {self.valid_from.valid_from} - {self.valid_to.valid_to} as it overlaps with a current record or records' | ||
| ) | ||
| exception_message = f'This record will result in duplicate configuration elements being returned for the date range {self.valid_from.valid_from} - {self.valid_to.valid_to} as it overlaps with a current record or records' | ||
| from reporting.utils import validate_overlapping_records |
dleard
force-pushed
the
chore/270-schema-models
branch
from
bfc71f9 to
b203bdf
Compare
Adding the models necessary to fetch schemas at different levels of data object.
These models will need fkey relationships to a configuration (valid_from, valid_to) as they need to be versioned & retrievable by when they were/are vaild.
The activitySchema will hold any schema data necessary when we only know the activity. (This will mean it is sometimes empty if no activity-level data needs to be collected)
The activitySourceTypeSchema data will be dynamically added to the activity schema by an API service using the configuration elements & user input