ListField not validated on append

[lafrech]

I'm not sure whether this is expected behavior or not.

One can set a validator on a ListField, but it is used only when setting values, not when appending.

Here's a trivial example:

        @db_instance.register
        class Doc(Document):
            l = fields.ListField(
                fields.IntField,
                validate=NoneOf(([1, 2, 3],))
            )

        d = Doc()

        with pytest.raises(ValidationError):
            d.l = [1, 2, 3]

        d.l = [1, 2]

        # The test fails here. No Exception raised.
        with pytest.raises(ValidationError):
            d.l.append(3)

        # However, an exception is raised here.
        with pytest.raises(ValidationError):
            d.l = d.l

This does not seem consistent. Assuming this is a bug/shortcoming, I'm not sure how I would address this.

My use case is a custom validator that checks a subitem is unique in a list of embedded documents: I have a ListField of EmbeddedField and I want to be sure that there is not two embedded documents with the same value for some specific field (or list of fields).

The validator is applied to the list, as it can obviously not be applied to the embedded documents individually.

Worse, I need it to be applied not only when an embedded document is appended to the list, but also when an embedded document in the list is mutated. If the latter is not feasible, I guess I can work this around by making the field read_only in my API. Better than nothing.

[lafrech]

Unless I'm missing something, this is a really blocking issue, as this allows to set invalid data that will not be checked even at commit time.

I could modify List to keep a reference to its parent field and call

    self._parent_field._validate(self)

in set_modified to ensure the field is validated each time the list is modified.

However, this is not satisfying because

• The validation error will be raised after the data is modified, so if the error is caught, the data is still wrong.

• This does not address the mutation of a List element. Not sure how this could be addressed.

Or should we live with it? In this case, it would be nice to at least validate the data on commit. I dug a bit into this and I'm afraid it's not easier.

What I can do is add pre_insert / pre_update methods in Doc where I manually validate the values:

self.Schema().fields['l'].validate(self.l)

Is there a better workaround for this?

[touilleMan]

You're totally right, this is a serious issue.

Worse, I need it to be applied not only when an embedded document is appended to the list, but also when an embedded document in the list is mutated. If the latter is not feasible,

This should be feasible with umongo !

I could modify List to keep a reference to its parent field and call

This seems the right way to do it for me (I fact it was like this in the old versions of umongo, but get removed because... stuff)

I'm thinking of a small rollback system to handle this:

• each BaseDataObject got a link on it parent (which means there should be swallow copy when reusing them)
• once a modification order received, the BaseDataObject runs it own validation and raise exception if needed (this is what is done in current version)
• when the new data has been validated, it set it into it field and call the parent's validator
• the parent run validation and call it own parent validator
• this goes up to the Document which runs it own validator
• if one of the validators got an error, they call on their child a _revert method. This call goes down through the parents to the BaseDataObject.
• The BaseDataObject has kept track of it change and can easily revert it (if a field is modified keep the old version, if a list is modified, keep the old list)

We can optimize this by only calling the parent validator if it is requested (i.e. almost never) Another solution would be to use a mechanism of publish/subscribe. This could be cleaner when BaseDataObject are used multiple time across documents/embedded documents, however this bring another dependency (or cumbersome code if we do it ourself).

[lafrech]

I'm glad to hear this should work. Indeed, I realized afterwards that even list elements mutations are catchable since these are not just objects but umongo fields (DictField being an exception to that again). That's great.

Regarding the method, I'm afraid I won't be of much help. I have no specific knowledge of publish/subscribe systems, so I can't provide any relevant advice about it. umongo is growing and it certainly makes sense to make things in a canonical way (I mean following a common pattern, be it with own code or using an external lib) rather than introducing hacks everywhere.

I don't think adding a dependency is an issue by itself. As long as it is reliable in time. Unless I need a very small subset, I find it better to depend on a code shared with other projects than rewriting it.

[lafrech]

~Related: del my_document.my_list[i] doesn't mark my_list field as modified.~ (Fixed in #195)

[lafrech]

This also affects DictField.update and such.

[lafrech]

In fact, reading @touilleMan's comment above, I'm realizing that validators other than field validators, such as schema-level validators are only executed when loading data on object instantiation (__init__ -> schema load), at least not when modifying a single field.

Doing so might be a little bit complicated and add a lot of overhead. We could decide we don't support custom validators such as schema validators.

However, I still think field validators as in OP should be completely supported so this bug still stands.

We could just do the change in the data object, call parent field validator, catch error and rollback the change in a finally clause. This could be simple. But it doesn't work for deeper nesting, like list of lists, or embedded fields,...

[zevaryx]

I think I'm having an issue related to this in one of my programs. I have the following code, and Roleping.is_modified isn't getting set as True in the following:

class Roleping(Document):
    """Roleping database object."""

    active = fields.BooleanField(default=True)
    role = fields.IntegerField(required=True)
    guild = fields.IntegerField(required=True)
    admin = fields.IntegerField(required=True)
    bypass = fields.DictField()
    created_at = fields.DateTimeField(default=datetime.utcnow)

# in another file while modifying
roleping.bypass["users"].append(bypass.id)  # Does not set is_modified to True

Having a list inside of a dictionary doesn't flag the dictionary as having been updated