add ArrayField #197
add ArrayField #197
Conversation
timgraham
force-pushed
the
arrayfield
branch
2 times, most recently
from
e7088d1 to
6338305
Compare
timgraham
force-pushed
the
arrayfield
branch
4 times, most recently
from
6a97c8e to
8a7d407
Compare
| Specifies the underlying data type and behavior for the array. It | ||
| should be an instance of a subclass of |
There was a problem hiding this comment.
I'm assuming we cannot confirm embedded models until after the embedded model PR is merged.
There was a problem hiding this comment.
I plan to merge this first, then return to EMF and decide what to do.
django_mongodb/fields/array.py
Outdated
| if isinstance(value, list | tuple): | ||
| # Workaround for https://code.djangoproject.com/ticket/35982 | ||
| if isinstance(self.base_field, DecimalField): | ||
| return [self.base_field.get_db_prep_save(i, connection) for i in value] | ||
| return [self.base_field.get_db_prep_value(i, connection, prepared=False) for i in value] | ||
| return value |
There was a problem hiding this comment.
I see the ticket is now marked as resolved and the change will now work with get_db_prep_value. Is this fix now a part of 5.1 or we still need to hold onto this workaround?
There was a problem hiding this comment.
It's fixed in Django 5.2. (This PR still targets Django 5.0.)
django_mongodb/fields/array.py
Outdated
| "$eq": [ | ||
| { | ||
| "$cond": { | ||
| "if": {"$eq": [lhs_mql, None]}, | ||
| "then": False, | ||
| "else": {"$setIsSubset": [value, lhs_mql]}, | ||
| } | ||
| }, | ||
| True, | ||
| ] |
There was a problem hiding this comment.
could we not use $all here?
Here's the equivalent:
{ "$all": value }There was a problem hiding this comment.
I tried it but where does lhs_mql go? return {lhs_mql: {"$all": value}} gives, for example, {'$match': {'$expr': {'$field': {'$all': [{'$literal': 2}]}}}} but MongoDB reports "Unrecognized expression '$field'". I think $all can only be used for find, not in aggregate.
django_mongodb/forms/array.py
Outdated
| super().__init__(**kwargs) | ||
| if min_length is not None: | ||
| self.min_length = min_length | ||
| self.validators.append(ArrayMinLengthValidator(int(min_length))) |
There was a problem hiding this comment.
this can come as anything other than int | None I or is this just an easy way to raise a type validation with int casting?
There was a problem hiding this comment.
I guess so. This doesn't look like typical Django code. As I recall, this code was merged a bit hastily as part of a massive PR during a sprint in 2014 without my careful review. I haven't reviewed the forms side of this PR carefully yet, and while doing so, I'll remove these casts (and probably propose they be removed upstream as well, considering there are no test failures without them).
edit: upon further examination, it was copied from CharField, and yes, it's for type validation: https://code.djangoproject.com/ticket/20440
django_mongodb/forms/array.py
Outdated
| def validate(self, value): | ||
| super().validate(value) | ||
| errors = [] | ||
| for index, item in enumerate(value): | ||
| try: | ||
| self.base_field.validate(item) | ||
| except ValidationError as error: | ||
| errors.append( | ||
| prefix_validation_error( | ||
| error, | ||
| prefix=self.error_messages["item_invalid"], | ||
| code="item_invalid", | ||
| params={"nth": index + 1}, | ||
| ) | ||
| ) | ||
| if errors: | ||
| raise ValidationError(errors) |
There was a problem hiding this comment.
Looks like the validate functions here share a lot with the field class version. Any chance we could make a low-pri task for a mixin class?
There was a problem hiding this comment.
There are subtle differences. For example, the signature of validate() is different between form fields and model fields.
I wouldn't advise using mixins to share functionality between model fields and form fields because django.forms and django.db.models (and this project's corresponding modules) are loosely coupled.
Based on
django.contrib.postgres.fields.ArrayField.