Skip to content

Conversation

@ColeDCrawford
Copy link
Contributor

This PR sets up a test Django project implementing EDTFField and adding Django integration tests. It also adds an additional class attribute direct_input_field to EDTFField so users have the option to provide an EDTF string directly instead of or in addition to using natural_field_text. If both natural_field_text and direct_input_field are provided, direct_input_text supersedes it for date generation; natural_field_text can still be used as a label. If only direct_input_field is provided, then natural_field_text is filled with the EDTF string.

This PR should be merged after the pytest PR.

- Use parameterization in pytest to simplify many test cases by wrapping the input and expected values in tuples
- No need for unittest or class wrappers
- There is a legitimately failing test in parser/tests.py - code change needed in parser_classes to handle this I think
- natlang/tests.py will need to be updated to match the new spec, as will the whole text_to_edtf() function
The max and min functions now use a generator expression to filter out infinite values unless they are directly relevant to the calculation; if inf or -inf are found, they are returned instead of doing a comparison.
In Django 3.0, "support for the context argument of Field.from_db_value() and Expression.convert_value() is removed": https://github.com/django/django/blob/91a4b9a8ec2237434f06866f39c7977e889aeae6/docs/releases/3.0.txt#L641-L642
Previously, the Django field could directly take an EDTF string, only a natural language string that was then parsed and turned into EDTF.
Django 4 test project for now
Ignore SQLlite local database
This basic Django app shows how a user could create a model using the EDTFField and store data in it. The integration tests check that the EDTFField and associated fields (date_edtf_direct and date_display, in this case) work correctly.

There is a weird issue in test_date_display() where if we use an instance variable (self.event1, self.event2) the event.date_display property is available, but if we retrieve the object from the database it is not. I tried using TestEvent.objects.create() as well as the current method (make and then save an instance to no effect).

CI is set up to run the Django integration tests after Pytest. We could move to using pytest/django-pytest for these tests as well
@aweakley
Copy link
Member

Thanks very much for this.

I've been looking at your comment on fbf4262 about the failing test.

I found that if I move the definition of date_display on the model down below the definition of date_edtf then it works. I think the cause is that here: https://github.com/django/django/blob/d26c8838d0a447d5cba03a0b4eebd5fc2d27e9df/django/db/models/sql/compiler.py#L1747 it's calling pre_save on each field in order and saving the result. So it's already stored the empty value of date_display before that value is populated by the pre_save of the date_edtf field.

In ImageField they do a similar thing to populate the width and height fields by attaching a signal handler to post_init. I wonder if we should move some of the pre_save processing into something like that?

@aweakley
Copy link
Member

... oh, post_init isn't the full story - the update-fields method is also called by the __set__ method of the file descriptor - that makes more sense, I was wondering how post_init would capture every change.

@aweakley
Copy link
Member

Something like this maybe?

diff --git a/edtf/fields.py b/edtf/fields.py
index bbccbcf..325b5aa 100644
--- a/edtf/fields.py
+++ b/edtf/fields.py
@@ -3,12 +3,14 @@ try:
 except:
     import pickle
 
-from django.db import models
 from django.core.exceptions import FieldDoesNotExist
+from django.db import models
+from django.db.models import signals
+from django.db.models.query_utils import DeferredAttribute
 
 from edtf import parse_edtf, EDTFObject
-from edtf.natlang import text_to_edtf
 from edtf.convert import struct_time_to_date, struct_time_to_jd
+from edtf.natlang import text_to_edtf
 
 DATE_ATTRS = (
     'lower_strict',
@@ -17,6 +19,20 @@ DATE_ATTRS = (
     'upper_fuzzy',
 )
 
+class EDTFFieldDescriptor(DeferredAttribute):
+    """
+    Descriptor for the EDTFField's attribute on the model instance.
+    This updates the dependent fields each time this value is set.
+    """
+
+    def __set__(self, instance, value):
+        # First set the value we are given
+        instance.__dict__[self.field.attname] = value
+        # `update_values` may provide us with a new value to set
+        edtf = self.field.update_values(instance, value)
+        if edtf != value:
+            instance.__dict__[self.field.attname] = edtf
+
 
 class EDTFField(models.CharField):
 
@@ -40,6 +56,7 @@ class EDTFField(models.CharField):
         super(EDTFField, self).__init__(verbose_name, name, **kwargs)
 
     description = "A field for storing complex/fuzzy date specifications in EDTF format."
+    descriptor_class = EDTFFieldDescriptor
 
     def deconstruct(self):
         name, path, args, kwargs = super(EDTFField, self).deconstruct()
@@ -88,14 +105,14 @@ class EDTFField(models.CharField):
             return pickle.dumps(value)
         return value
 
-    def pre_save(self, instance, add):
+    def update_values(self, instance, *args, **kwargs):
         """
         Updates the EDTF value from either the natural_text_field, which is parsed
         with text_to_edtf() and is used for display, or falling back to the direct_input_field,
         which allows directly providing an EDTF string. If one of these provides a valid EDTF object,
         then set the date values accordingly.
         """
-    
+
         # Get existing value to determine if update is needed
         existing_value = getattr(instance, self.attname, None)
         direct_input = getattr(instance, self.direct_input_field, None)
@@ -123,10 +140,6 @@ class EDTFField(models.CharField):
             # TODO: if both direct_input and natural_text are cleared, should we throw an error?
             edtf = existing_value
 
-        # Update the actual EDTF field in the model if there is a change
-        if edtf != existing_value:
-            setattr(instance, self.attname, edtf)
-
         # Process and update related date fields based on the EDTF object
         for attr in DATE_ATTRS:
             field_attr = f"{attr}_field"
@@ -151,3 +164,12 @@ class EDTFField(models.CharField):
                 else:
                     setattr(instance, g, None)
         return edtf
+
+    def contribute_to_class(self, cls, name, **kwargs):
+        super().contribute_to_class(cls, name, **kwargs)
+        # Attach update_values so that dependent fields declared
+        # after their corresponding edtf field don't stay cleared by
+        # Model.__init__, see Django bug #11196.
+        # Only run post-initialization values update on non-abstract models
+        if not cls._meta.abstract:
+            signals.post_init.connect(self.update_values, sender=cls)
diff --git a/edtf_django_tests/edtf_integration/tests.py b/edtf_django_tests/edtf_integration/tests.py
index de54d64..e0b80f9 100644
--- a/edtf_django_tests/edtf_integration/tests.py
+++ b/edtf_django_tests/edtf_integration/tests.py
@@ -73,9 +73,8 @@ class TestEventModelTests(TestCase):
         In the future, a more sophisticated natural language parser could be used to generate
         a human readable date from the EDTF input.
         """
-        # why does this fail??
-        # event = TestEvent.objects.get(date_edtf_direct="2020-03-15/2020-04-15")
-        # self.assertEqual(event.date_display, "2020-03-15/2020-04-15")
+        event = TestEvent.objects.get(date_edtf_direct="2020-03-15/2020-04-15")
+        self.assertEqual(event.date_display, "2020-03-15/2020-04-15")
 
         self.assertEqual(self.event1.date_display, "2020-03-15/2020-04-15")
         self.assertEqual(self.event2.date_display, "2021-05-06")
@@ -93,4 +92,4 @@ class TestEventModelTests(TestCase):
         self.assertGreater(self.event2.date_edtf, self.event3.date_edtf, "2021-05-06 is greater than 2019-11")
 
         # less than
-        self.assertLess(self.event3.date_edtf, self.event2.date_edtf, "2019-11 is less than 2021-05-06")
\ No newline at end of file
+        self.assertLess(self.event3.date_edtf, self.event2.date_edtf, "2019-11 is less than 2021-05-06")

Ensure that `EDTFField` properly updates related fields whenever it changes inspired by ImageField.

- Use EDTFFieldDescriptorClass as a descriptor for EDTFField. This inherits from DeferredAttribute and handles getting, setting, and updating values. Whenever the field value is set, additional logic is processed to potentially update the field again based on other fields.
- update_values() replaces pre_save() to better handle updates/dependencies when EDTFField value changes
- contribute_to_class() attaches update_values() to the `post_init` signal

These changes should make the field updates more stable and (not reliant on definition order in models using EDTFField).

Thanks for the suggestion @aweakley ixc#47 (comment)

Co-Authored-By: aweakley <224316+aweakley@users.noreply.github.com>
@aweakley
Copy link
Member

Great, thanks very much.

@aweakley aweakley merged commit 613ccf5 into ixc:v5 May 12, 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