From ebe9416abd18977ecd73ea5e09b62527b08e1a66 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 10 Nov 2023 10:39:38 -0700 Subject: [PATCH 1/3] Don't leak open server PID file Running with PYTHONWARNINGS=all reveals: ``` /lib/python3.9/site-packages/kolibri/utils/server.py:881: ResourceWarning: unclosed file <_io.TextIOWrapper name='/server.pid' mode='r' encoding='UTF-8'> pid_file_lines = open(filename, "r").readlines() ResourceWarning: Enable tracemalloc to get the object allocation traceback ``` --- kolibri/utils/server.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kolibri/utils/server.py b/kolibri/utils/server.py index cfaab345f7a..0770d98a2b8 100644 --- a/kolibri/utils/server.py +++ b/kolibri/utils/server.py @@ -890,7 +890,8 @@ def _read_pid_file(filename): return None, None, None, STATUS_STOPPED try: - pid_file_lines = open(filename, "r").readlines() + with open(filename, "r") as f: + pid_file_lines = f.readlines() pid, port, zip_port, status = pid_file_lines pid = int(pid.strip()) port = int(port.strip()) if port.strip() else None From 2c8a828ef34c50171daefe0b89ed850272a21943 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 10 Nov 2023 11:00:56 -0700 Subject: [PATCH 2/3] Don't leak open version file Running with PYTHONWARNINGS=all reveals: ``` /kolibri/utils/main.py:100: ResourceWarning: unclosed file <_io.TextIOWrapper name='/.data_version' mode='r' encoding='UTF-8'> version = open(version_file(), "r").read() ``` --- kolibri/utils/main.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kolibri/utils/main.py b/kolibri/utils/main.py index 01960a0f201..d02a4bd2eb3 100644 --- a/kolibri/utils/main.py +++ b/kolibri/utils/main.py @@ -97,7 +97,8 @@ def conditional_backup(kolibri_version, version_file_contents): def get_version(): try: - version = open(version_file(), "r").read() + with open(version_file(), "r") as f: + version = f.read() return version.strip() if version else "" except IOError: return "" From 8c214f5d4065eb94b22e99bddc247a3e940610ad Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 10 Nov 2023 10:57:51 -0700 Subject: [PATCH 3/3] Explicitly set on_delete for ForeignKey fields Running with PYTHONWARNINGS=all reveals several deprecation warnings that `on_delete` must be specified for `ForeignKey` fields: ``` RemovedInDjango20Warning: on_delete will be a required arg for ForeignKey in Django 2.0. Set it to models.CASCADE on models and in existing migrations if you want to maintain the current default behavior. See https://docs.djangoproject.com/en/1.11/ref/models/fields/#django.db.models.ForeignKey.on_delete ``` --- kolibri/core/analytics/models.py | 4 +-- kolibri/core/auth/models.py | 17 +++++++++--- kolibri/core/bookmarks/models.py | 2 +- kolibri/core/content/base_models.py | 29 ++++++++++++++++----- kolibri/core/content/models.py | 4 ++- kolibri/core/discovery/models.py | 2 +- kolibri/core/exams/models.py | 36 +++++++++++++++++++++----- kolibri/core/lessons/models.py | 34 +++++++++++++++++++----- kolibri/core/logger/models.py | 40 +++++++++++++++++++++-------- 9 files changed, 127 insertions(+), 41 deletions(-) diff --git a/kolibri/core/analytics/models.py b/kolibri/core/analytics/models.py index b09ba870959..cf89efff529 100644 --- a/kolibri/core/analytics/models.py +++ b/kolibri/core/analytics/models.py @@ -24,8 +24,8 @@ class PingbackNotificationDismissed(models.Model): permissions = IsOwn() - user = models.ForeignKey(FacilityUser) - notification = models.ForeignKey(PingbackNotification) + user = models.ForeignKey(FacilityUser, on_delete=models.CASCADE) + notification = models.ForeignKey(PingbackNotification, on_delete=models.CASCADE) class Meta: unique_together = (("user", "notification"),) diff --git a/kolibri/core/auth/models.py b/kolibri/core/auth/models.py index 6a958d78fcd..95a78ef1ccb 100644 --- a/kolibri/core/auth/models.py +++ b/kolibri/core/auth/models.py @@ -1034,7 +1034,12 @@ class Collection(AbstractFacilityDataModel): name = models.CharField(max_length=100) parent = models.ForeignKey( - "self", null=True, blank=True, related_name="children", db_index=True + "self", + null=True, + blank=True, + related_name="children", + db_index=True, + on_delete=models.CASCADE, ) kind = models.CharField(max_length=20, choices=collection_kinds.choices) @@ -1225,11 +1230,15 @@ class Membership(AbstractFacilityDataModel): permissions = own | role | membership user = models.ForeignKey( - "FacilityUser", related_name="memberships", blank=False, null=False + "FacilityUser", + related_name="memberships", + blank=False, + null=False, + on_delete=models.CASCADE, ) # Note: "It's recommended you use mptt.fields.TreeForeignKey wherever you have a foreign key to an MPTT model. # https://django-mptt.github.io/django-mptt/models.html#treeforeignkey-treeonetoonefield-treemanytomanyfield - collection = TreeForeignKey("Collection") + collection = TreeForeignKey("Collection", on_delete=models.CASCADE) class Meta: unique_together = (("user", "collection"),) @@ -1320,7 +1329,7 @@ class Role(AbstractFacilityDataModel): ) # Note: "It's recommended you use mptt.fields.TreeForeignKey wherever you have a foreign key to an MPTT model. # https://django-mptt.github.io/django-mptt/models.html#treeforeignkey-treeonetoonefield-treemanytomanyfield - collection = TreeForeignKey("Collection") + collection = TreeForeignKey("Collection", on_delete=models.CASCADE) kind = models.CharField(max_length=26, choices=role_kinds.choices) class Meta: diff --git a/kolibri/core/bookmarks/models.py b/kolibri/core/bookmarks/models.py index 1dfef2e24e0..460a26f77f1 100644 --- a/kolibri/core/bookmarks/models.py +++ b/kolibri/core/bookmarks/models.py @@ -15,7 +15,7 @@ class Bookmark(AbstractFacilityDataModel): content_id = UUIDField(blank=True, null=True) channel_id = UUIDField(blank=True, null=True) contentnode_id = UUIDField() - user = models.ForeignKey(FacilityUser, blank=False) + user = models.ForeignKey(FacilityUser, blank=False, on_delete=models.CASCADE) created = models.DateTimeField(default=timezone.now, db_index=True) morango_model_name = "bookmark" diff --git a/kolibri/core/content/base_models.py b/kolibri/core/content/base_models.py index 8737ea38c61..d8c326a66fd 100644 --- a/kolibri/core/content/base_models.py +++ b/kolibri/core/content/base_models.py @@ -99,7 +99,12 @@ class ContentNode(MPTTModel): id = UUIDField(primary_key=True) parent = TreeForeignKey( - "self", null=True, blank=True, related_name="children", db_index=True + "self", + null=True, + blank=True, + related_name="children", + db_index=True, + on_delete=models.CASCADE, ) license_name = models.CharField(max_length=50, null=True, blank=True) license_description = models.TextField(null=True, blank=True) @@ -127,7 +132,9 @@ class ContentNode(MPTTModel): author = models.CharField(max_length=200, blank=True) kind = models.CharField(max_length=200, choices=content_kinds.choices, blank=True) available = models.BooleanField(default=False) - lang = models.ForeignKey("Language", blank=True, null=True) + lang = models.ForeignKey( + "Language", blank=True, null=True, on_delete=models.CASCADE + ) # A JSON Dictionary of properties to configure loading, rendering, etc. the file options = JSONField(default={}, blank=True, null=True) @@ -169,12 +176,18 @@ class File(models.Model): id = UUIDField(primary_key=True) # The foreign key mapping happens here as many File objects can map onto a single local file - local_file = models.ForeignKey("LocalFile", related_name="files") - contentnode = models.ForeignKey("ContentNode", related_name="files") + local_file = models.ForeignKey( + "LocalFile", related_name="files", on_delete=models.CASCADE + ) + contentnode = models.ForeignKey( + "ContentNode", related_name="files", on_delete=models.CASCADE + ) preset = models.CharField( max_length=150, choices=format_presets.choices, blank=True ) - lang = models.ForeignKey("Language", blank=True, null=True) + lang = models.ForeignKey( + "Language", blank=True, null=True, on_delete=models.CASCADE + ) supplementary = models.BooleanField(default=False) thumbnail = models.BooleanField(default=False) priority = models.IntegerField(blank=True, null=True, db_index=True) @@ -209,7 +222,9 @@ class AssessmentMetaData(models.Model): """ id = UUIDField(primary_key=True) - contentnode = models.ForeignKey("ContentNode", related_name="assessmentmetadata") + contentnode = models.ForeignKey( + "ContentNode", related_name="assessmentmetadata", on_delete=models.CASCADE + ) # A JSON blob containing a serialized list of ids for questions that the assessment can present. assessment_item_ids = JSONField(default=[]) # Length of the above assessment_item_ids for a convenience lookup. @@ -241,7 +256,7 @@ class ChannelMetadata(models.Model): last_updated = DateTimeTzField(null=True) # Minimum version of Kolibri that this content database is compatible with min_schema_version = models.CharField(max_length=50) - root = models.ForeignKey("ContentNode") + root = models.ForeignKey("ContentNode", on_delete=models.CASCADE) class Meta: abstract = True diff --git a/kolibri/core/content/models.py b/kolibri/core/content/models.py index 2e0e16b8827..7eea4a65a04 100644 --- a/kolibri/core/content/models.py +++ b/kolibri/core/content/models.py @@ -468,7 +468,9 @@ class ContentRequest(models.Model): """ id = UUIDField(primary_key=True, default=_hex_uuid_str) - facility = models.ForeignKey(Facility, related_name="content_requests") + facility = models.ForeignKey( + Facility, related_name="content_requests", on_delete=models.CASCADE + ) # the source model's `morango_model_name` that initiated the request: # - for user-initiated requests it should be `facilityuser` diff --git a/kolibri/core/discovery/models.py b/kolibri/core/discovery/models.py index e0a405772e3..9ecaa050040 100644 --- a/kolibri/core/discovery/models.py +++ b/kolibri/core/discovery/models.py @@ -277,7 +277,7 @@ def allow_migrate(self, db, app_label, model_name=None, **hints): class PinnedDevice(models.Model): instance_id = UUIDField(blank=False) - user = models.ForeignKey(FacilityUser, blank=False) + user = models.ForeignKey(FacilityUser, blank=False, on_delete=models.CASCADE) created = models.DateTimeField(default=timezone.now, db_index=True) permissions = IsOwn() diff --git a/kolibri/core/exams/models.py b/kolibri/core/exams/models.py index 76ef735e1b6..05eba071bb0 100644 --- a/kolibri/core/exams/models.py +++ b/kolibri/core/exams/models.py @@ -112,10 +112,18 @@ class Exam(AbstractFacilityDataModel): # who creates them in the context of their class, this stores that relationship but does # not assign exam itself to the class - for that see the ExamAssignment model. collection = models.ForeignKey( - Collection, related_name="exams", blank=False, null=False + Collection, + related_name="exams", + blank=False, + null=False, + on_delete=models.CASCADE, ) creator = models.ForeignKey( - FacilityUser, related_name="exams", blank=False, null=True + FacilityUser, + related_name="exams", + blank=False, + null=True, + on_delete=models.CASCADE, ) # To be set True when the quiz is first set to active=True @@ -206,12 +214,26 @@ class ExamAssignment(AbstractFacilityDataModel): ) | UserCanReadExamAssignmentData() ) - exam = models.ForeignKey(Exam, related_name="assignments", blank=False, null=False) + exam = models.ForeignKey( + Exam, + related_name="assignments", + blank=False, + null=False, + on_delete=models.CASCADE, + ) collection = models.ForeignKey( - Collection, related_name="assigned_exams", blank=False, null=False + Collection, + related_name="assigned_exams", + blank=False, + null=False, + on_delete=models.CASCADE, ) assigned_by = models.ForeignKey( - FacilityUser, related_name="assigned_exams", blank=False, null=True + FacilityUser, + related_name="assigned_exams", + blank=False, + null=True, + on_delete=models.CASCADE, ) def pre_save(self): @@ -282,8 +304,8 @@ class IndividualSyncableExam(AbstractFacilityDataModel): morango_model_name = "individualsyncableexam" - user = models.ForeignKey(FacilityUser) - collection = models.ForeignKey(Collection) + user = models.ForeignKey(FacilityUser, on_delete=models.CASCADE) + collection = models.ForeignKey(Collection, on_delete=models.CASCADE) exam_id = models.UUIDField() serialized_exam = JSONField() diff --git a/kolibri/core/lessons/models.py b/kolibri/core/lessons/models.py index 4c0447f3e04..87149a18c31 100644 --- a/kolibri/core/lessons/models.py +++ b/kolibri/core/lessons/models.py @@ -57,11 +57,19 @@ class Lesson(AbstractFacilityDataModel): # The Classroom-type Collection for which the Lesson is created collection = models.ForeignKey( - Collection, related_name="lessons", blank=False, null=False + Collection, + related_name="lessons", + blank=False, + null=False, + on_delete=models.CASCADE, ) created_by = models.ForeignKey( - FacilityUser, related_name="lessons_created", blank=False, null=True + FacilityUser, + related_name="lessons_created", + blank=False, + null=True, + on_delete=models.CASCADE, ) date_created = DateTimeTzField(default=local_now, editable=False) @@ -130,13 +138,25 @@ class LessonAssignment(AbstractFacilityDataModel): ) lesson = models.ForeignKey( - Lesson, related_name="lesson_assignments", blank=False, null=False + Lesson, + related_name="lesson_assignments", + blank=False, + null=False, + on_delete=models.CASCADE, ) collection = models.ForeignKey( - Collection, related_name="assigned_lessons", blank=False, null=False + Collection, + related_name="assigned_lessons", + blank=False, + null=False, + on_delete=models.CASCADE, ) assigned_by = models.ForeignKey( - FacilityUser, related_name="assigned_lessons", blank=False, null=True + FacilityUser, + related_name="assigned_lessons", + blank=False, + null=True, + on_delete=models.CASCADE, ) def __str__(self): @@ -214,8 +234,8 @@ class IndividualSyncableLesson(AbstractFacilityDataModel): morango_model_name = "individualsyncablelesson" - user = models.ForeignKey(FacilityUser) - collection = models.ForeignKey(Collection) + user = models.ForeignKey(FacilityUser, on_delete=models.CASCADE) + collection = models.ForeignKey(Collection, on_delete=models.CASCADE) lesson_id = models.UUIDField() serialized_lesson = JSONField() diff --git a/kolibri/core/logger/models.py b/kolibri/core/logger/models.py index e64a21df62a..0ddc8fc8d60 100644 --- a/kolibri/core/logger/models.py +++ b/kolibri/core/logger/models.py @@ -110,7 +110,9 @@ class ContentSessionLog(BaseLogModel): # Morango syncing settings morango_model_name = "contentsessionlog" - user = models.ForeignKey(FacilityUser, blank=True, null=True) + user = models.ForeignKey( + FacilityUser, blank=True, null=True, on_delete=models.CASCADE + ) content_id = UUIDField(db_index=True) visitor_id = models.UUIDField(blank=True, null=True) channel_id = UUIDField(blank=True, null=True) @@ -139,7 +141,7 @@ class ContentSummaryLog(BaseLogModel): # Morango syncing settings morango_model_name = "contentsummarylog" - user = models.ForeignKey(FacilityUser) + user = models.ForeignKey(FacilityUser, on_delete=models.CASCADE) content_id = UUIDField(db_index=True) channel_id = UUIDField(blank=True, null=True) start_timestamp = DateTimeTzField() @@ -172,7 +174,7 @@ class UserSessionLog(BaseLogModel): # Morango syncing settings morango_model_name = "usersessionlog" - user = models.ForeignKey(FacilityUser) + user = models.ForeignKey(FacilityUser, on_delete=models.CASCADE) channels = models.TextField(blank=True) start_timestamp = DateTimeTzField(default=local_now) last_interaction_timestamp = DateTimeTzField(null=True, blank=True) @@ -226,9 +228,11 @@ class MasteryLog(BaseLogModel): # Morango syncing settings morango_model_name = "masterylog" - user = models.ForeignKey(FacilityUser) + user = models.ForeignKey(FacilityUser, on_delete=models.CASCADE) # Every MasteryLog is related to the single summary log for the user/content pair - summarylog = models.ForeignKey(ContentSummaryLog, related_name="masterylogs") + summarylog = models.ForeignKey( + ContentSummaryLog, related_name="masterylogs", on_delete=models.CASCADE + ) # The MasteryLog records the mastery criterion that has been specified for the user. # It is recorded here to prevent this changing in the middle of a user's engagement # with an assessment. @@ -288,7 +292,9 @@ class BaseAttemptLog(BaseLogModel): # A JSON Array with a sequence of JSON objects that describe the history of interaction of the user # with this assessment item in this attempt. interaction_history = JSONField(default=[], blank=True) - user = models.ForeignKey(FacilityUser, blank=True, null=True) + user = models.ForeignKey( + FacilityUser, blank=True, null=True, on_delete=models.CASCADE + ) error = models.BooleanField(default=False) class Meta: @@ -305,9 +311,15 @@ class AttemptLog(BaseAttemptLog): # Which mastery log was this attemptlog associated with? masterylog = models.ForeignKey( - MasteryLog, related_name="attemptlogs", blank=True, null=True + MasteryLog, + related_name="attemptlogs", + blank=True, + null=True, + on_delete=models.CASCADE, + ) + sessionlog = models.ForeignKey( + ContentSessionLog, related_name="attemptlogs", on_delete=models.CASCADE ) - sessionlog = models.ForeignKey(ContentSessionLog, related_name="attemptlogs") def infer_dataset(self, *args, **kwargs): return self.cached_related_dataset_lookup("sessionlog") @@ -322,9 +334,11 @@ class ExamLog(BaseLogModel): morango_model_name = "examlog" # Identifies the exam that this is for. - exam = models.ForeignKey(Exam, related_name="examlogs", blank=False, null=False) + exam = models.ForeignKey( + Exam, related_name="examlogs", blank=False, null=False, on_delete=models.CASCADE + ) # Identifies which user this log summarizes interactions for. - user = models.ForeignKey(FacilityUser) + user = models.ForeignKey(FacilityUser, on_delete=models.CASCADE) # Is this exam open for engagement, or is it closed? # Used to end user engagement with an exam when it has been deactivated. closed = models.BooleanField(default=False) @@ -346,7 +360,11 @@ class ExamAttemptLog(BaseAttemptLog): morango_model_name = "examattemptlog" examlog = models.ForeignKey( - ExamLog, related_name="attemptlogs", blank=False, null=False + ExamLog, + related_name="attemptlogs", + blank=False, + null=False, + on_delete=models.CASCADE, ) # We have no session logs associated with ExamLogs, so we need to record the channel and content # ids here