schema checks and unit tests. #3

WaVEV · 2024-12-07T17:35:19Z

I added the schema checks in get_transform, I didn't find any other place to put it.

The code got a bit dirty if we use embedded field with jsonfield. This use case is strange but possible.
I couldn't find any other way to do this task without breaking JSON.

django_mongodb/compiler.py

timgraham

Sorry for the delay in looking at this. Let's focus on the field validation (first commit) and move the querying bits to another PR.

timgraham · 2024-12-24T14:51:10Z

tests/model_fields_/test_embedded_model.py

@@ -82,18 +92,10 @@ def test_pre_save(self):
        self.assertEqual(obj.simple.auto_now_add, auto_now_add)
        self.assertGreater(obj.simple.auto_now, auto_now_two)

-    def test_foreign_key_in_embedded_object(self):


Why the deleted test?

Inside of embedded object we cannot have foreign keys, If we want to support it, I think I have to add some validations in the try_transform. Maybe we have to raise an exception ?

I agree; Let's raise an exception for now if a foreignkey is placed inside an embedded object. Otherwise, devs may try and it will fail.

The new test expects an exception. 😄

timgraham · 2024-12-24T14:51:21Z

tests/model_fields_/models.py

@@ -35,11 +35,12 @@ class DecimalParent(models.Model):

 class EmbeddedModelFieldModel(models.Model):
    simple = EmbeddedModelField("EmbeddedModel", null=True, blank=True)
-    decimal_parent = EmbeddedModelField(DecimalParent, null=True, blank=True)
+    decimal_parent = EmbeddedModelField(DecimalKey, null=True, blank=True)


Why the change?

Foreign key aren't supported in embedded fields, So I decided to change this fields. (Decimal parent has a FK to another table)

timgraham · 2024-12-24T14:51:38Z

django_mongodb/fields/embedded_model.py



 class KeyTransformFactory:
-    def __init__(self, key_name):
+    def __init__(self, key_name, ref_field=None):


Should ref_field be a required arg?

🤔 let me check, but I think you are right, I made some change in refactor to avoid nullability in ref_field.

WaVEV · 2024-12-30T02:42:38Z

Sorry for the delay in looking at this. Let's focus on the field validation (first commit) and move the querying bits to another PR.

I reorder the commits to have the first 5 for validation and the other 2 for testing.

timgraham · 2024-12-30T19:07:16Z

I rebased embedded-model-field branch on main, so if you rebase your branch on that, it should fix the issue.

timgraham · 2024-12-31T00:15:21Z

Refresh my memory... we can't support foreign key because of the double underscore lookup syntax collision, correct? In that case, I'm wondering if we'll need to revisit how we do querying in order to allow foreign key (even if it's a future task) in embedded documents. (I'm not sure whether or not we need to support it... have to check with Jib + team.)

WaVEV · 2024-12-31T00:57:23Z

Refresh my memory... we can't support foreign key because of the double underscore lookup syntax collision, correct? In that case, I'm wondering if we'll need to revisit how we do querying in order to allow foreign key (even if it's a future task) in embedded documents. (I'm not sure whether or not we need to support it... have to check with Jib + team.)

🤔 Yes, the problem is when Django interprets the query. The setup_joins function handles it, but it always treats the __ as joins (if it’s not a lookup). So, if a join comes before a path query into the embedded field, it works, but not the reverse (a join using an embedded field's field) because a different flow handles the query interpretation. If we want to support this kind of behavior, we should treat the embedded fields as fake-joins.

I don’t know if we can define a foreign key as a field in a JSONField. I don’t think so, but if it’s possible, we should take a look.

timgraham · 2024-12-31T01:34:48Z

No foreign key in JSON. It's always just "data."

See Djongo's EmbeddedField API for an alternate query syntax we could consider.

WaVEV · 2024-12-31T01:59:03Z

No foreign key in JSON. It's always just "data."

See Djongo's EmbeddedField API for an alternate query syntax we could consider.

🤔 This changes things a bit, I didn’t know this kind of syntax existed 😬. I have two questions:

In Django, filtering usually works with an argument structured as column_field + lookup, and the value is just the value to filter by. But here, we have a mixed approach where the value part includes both field + value, while the argument name is embedded_field + lookup. Is that correct?
The tests I wrote are based on the other format, so they’re not valid for this one.
Also, even though I don’t know how to implement a foreign field inside an embedded field, how would this syntax support such queries? I can think of two possible options:

entries = Entry.objects.filter(embedded_field__foreign_field__name__startswith={'name': 'Beatles'})
entries = Entry.objects.filter(embedded_field__startswith={'foreign_field__name': 'Beatles'})
Which one (if either) would we use?

Jibola · 2025-01-03T14:05:04Z

Refresh my memory... we can't support foreign key because of the double underscore lookup syntax collision, correct? In that case, I'm wondering if we'll need to revisit how we do querying in order to allow foreign key (even if it's a future task) in embedded documents. (I'm not sure whether or not we need to support it... have to check with Jib + team.)

I don't think it needs to be supported as I'd rather we remove any opportunities to promote the ForeignKey field usage; however, having a task to investigate its work sounds just fine to me.

Jibola · 2025-01-03T14:08:00Z

I added the schema checks in get_transform, I didn't find any other place to put it.

The code got a bit dirty if we use embedded field with jsonfield.

What would be a use-case you'd imagine? If someone is using an EmbeddedModel in a JSONField that feels counterintuitive.

I'm all for not supporting this usecase if it's too much work; work has already been done to support it, but if it's creating complications, you can take out the requirement. If someone is using EmbeddedField with JSONField, I'm hard-pressed to understand if that's worthwhile.

Jibola

Added some comments. I haven't reviewed all the test cases yet, but will come back to it today. :)

Jibola · 2025-01-03T14:46:31Z

django_mongodb/fields/embedded_model.py

+    @staticmethod
+    def _validate_embedded_field(_, model):


I don't think this needs to be a static method since you're only ever calling the instanced self version of the function.

Don't worry about this functionality. I'm instead going to amend my PR to use the same system check approach that ArrayField uses to disallow relational fields.

Jibola · 2025-01-03T14:58:13Z

tests/model_fields_/test_embedded_model.py

+        msg = "Address has no field named 'president'"
+        with self.assertRaisesMessage(FieldDoesNotExist, msg):
+            Book.objects.filter(author__address__city__president="NYC")


shouldn't this be city has no field name president? or address has no field named city.president?

🤔 I think you are right. I will check this out.

I used "Address.city has no field named 'president'" if you don't like it, let me know is a easy change

timgraham · 2025-01-01T22:40:03Z

tests/model_fields_/test_embedded_model.py

@@ -229,3 +242,62 @@ def test_ordering_grouping_by_sum(self):
            .order_by("sum")
        )
        self.assertQuerySetEqual(qs, [0, 2, 4, 6, 8, 10], operator.itemgetter("sum"))
+
+
+class SubqueryExistsTestCase(TestCase):


TestCase -> Tests (a TestCase is a base class that's meant to be inherited)

timgraham · 2025-01-03T15:38:29Z

django_mongodb/fields/embedded_model.py

+    @staticmethod
+    def _validate_embedded_field(_, model):


Don't worry about this functionality. I'm instead going to amend my PR to use the same system check approach that ArrayField uses to disallow relational fields.

WaVEV · 2025-01-04T05:49:24Z

I added the schema checks in get_transform, I didn't find any other place to put it.
The code got a bit dirty if we use embedded field with jsonfield.

What would be a use-case you'd imagine? If someone is using an EmbeddedModel in a JSONField that feels counterintuitive.

I'm all for not supporting this usecase if it's too much work; work has already been done to support it, but if it's creating complications, you can take out the requirement. If someone is using EmbeddedField with JSONField, I'm hard-pressed to understand if that's worthwhile.

I agree, the use case is unusual, and maybe we should suggest avoiding it. But as long as the embedded fields are fully typed, the user might have a reason for wanting semi-structured data inside embedded field.

WaVEV commented Dec 9, 2024

View reviewed changes

django_mongodb/compiler.py Outdated Show resolved Hide resolved

timgraham force-pushed the embedded-model-field branch from e2947b8 to dd65bb7 Compare December 13, 2024 20:32

WaVEV force-pushed the embedded-model-field-schema-checks branch from a752b20 to 9a8ffd5 Compare December 16, 2024 00:15

timgraham reviewed Dec 24, 2024

View reviewed changes

WaVEV force-pushed the embedded-model-field-schema-checks branch from bf71f56 to 86ac26a Compare December 30, 2024 02:40

WaVEV force-pushed the embedded-model-field-schema-checks branch from 86ac26a to 2d75b39 Compare December 30, 2024 03:07

timgraham force-pushed the embedded-model-field branch from dd65bb7 to 62cafa8 Compare December 30, 2024 18:57

WaVEV force-pushed the embedded-model-field-schema-checks branch from 2d75b39 to 4da45f2 Compare December 30, 2024 19:05

WaVEV added 7 commits December 30, 2024 20:41

schema checks and unit tests.

9a24853

Embedded field does not generates join ref counts.

7aa33c0

Fix clone issue when the output field takes arguments.

c97c7c3

refactor.

8dac716

Validate embedded model fields.

a4a8025

Add group by and order tests.

b6261c5

Add subquery and foreign field unit tests.

d6f7812

WaVEV force-pushed the embedded-model-field-schema-checks branch from 4da45f2 to d6f7812 Compare December 30, 2024 23:42

Jibola suggested changes Jan 3, 2025

View reviewed changes

timgraham reviewed Jan 3, 2025

View reviewed changes

WaVEV added 2 commits January 4, 2025 02:34

Edit class name.

93d684b

Fix exception message when a subfield does not have a selected field

0af7241

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema checks and unit tests. #3

schema checks and unit tests. #3

WaVEV commented Dec 7, 2024

timgraham left a comment

timgraham Dec 24, 2024

WaVEV Dec 29, 2024

Jibola Jan 3, 2025 •

edited

Loading

WaVEV Jan 3, 2025

timgraham Dec 24, 2024

WaVEV Dec 29, 2024

timgraham Dec 24, 2024

WaVEV Dec 29, 2024

WaVEV commented Dec 30, 2024

timgraham commented Dec 30, 2024

timgraham commented Dec 31, 2024

WaVEV commented Dec 31, 2024

timgraham commented Dec 31, 2024

WaVEV commented Dec 31, 2024

Jibola commented Jan 3, 2025

Jibola commented Jan 3, 2025

Jibola left a comment

Jibola Jan 3, 2025

timgraham Jan 3, 2025

Jibola Jan 3, 2025

WaVEV Jan 3, 2025

WaVEV Jan 4, 2025

timgraham Jan 1, 2025

timgraham Jan 3, 2025

WaVEV commented Jan 4, 2025

schema checks and unit tests. #3

Are you sure you want to change the base?

schema checks and unit tests. #3

Conversation

WaVEV commented Dec 7, 2024

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jibola Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaVEV commented Dec 30, 2024

timgraham commented Dec 30, 2024

timgraham commented Dec 31, 2024

WaVEV commented Dec 31, 2024

timgraham commented Dec 31, 2024

WaVEV commented Dec 31, 2024

Jibola commented Jan 3, 2025

Jibola commented Jan 3, 2025

Jibola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaVEV commented Jan 4, 2025

Jibola Jan 3, 2025 •

edited

Loading