Check if undefined fields are supplied on document

[gitaarik]

If an undefined field is supplied to a document instance, a FieldDoesNotExist Exception will be raised. This way, you will be notified when you provide a field that does not exist.

ATM when you do this:

MyDoc(undefined_field="test")

It will create a document instance for MyDoc. However, if undefined_field does not exist, the value test won't end up anywhere. You'll think the object is successfully created with the provided values, but actually it isn't.

To remedy this situation, an exception will be raised. This way you'll be noticed about your mistake in development. If the document instance is created dynamically on bases of an API call data for example, then you can catch the exeption and notify the API user that they have provided an undefined field.

[gitaarik]

Ok didn't read the contributing docs. Will look why these tests are failing, I hope it's not because they use undefined fields....

[rozza]

Will review for 0.9 as its a breaking change

[rozza]

Closing as cant be merged automatically. Feel free to comment on this ticket to reopen it.

@gitaarik Could you rebase onto master?

[gitaarik]

I rebased my fork and fixed some tests that were using undefined model fields. However, there's one test still failing and I'm a bit confused about what to do with it:
https://github.com/gitaarik/mongoengine/blob/master/tests/document/instance.py#L2329

FieldDoesNotExist: The field 'foo' does not exist on the document 'User'

What is exactly being tested here? Should user.foo still return True and user._data['foo'] should return what you've set? Since my change doesn't allow you to set data to an undefined field, this test seems to conflict it.

[rozza]

Its testing to make sure that setting a field doesnt overwrite the property. I think throwing the error is fine and we can now remove that test.

The pr looks a little funky now on the changes so may need rebasing as some other changes are present - eg progressive JPEG which is also in master - so not sure why the change is listed in your pr.

Also, can you add a regression test ensuring the exception is thrown

[gitaarik]

It should be good now. I get one error when running the tests though, but I think that might has to do with my system because I also get it when I run the tests in the original repo:

======================================================================
ERROR: test_text_indexes (tests.IndexesTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/Rik/dev/mongoengine/tests/document/indexes.py", line 751, in test_text_indexes
    indexes = Book.objects._collection.index_information()
  File "mongoengine/queryset/manager.py", line 37, in __get__
    queryset = queryset_class(owner, owner._get_collection())
  File "mongoengine/document.py", line 182, in _get_collection
    cls.ensure_indexes()
  File "mongoengine/document.py", line 604, in ensure_indexes
    drop_dups=drop_dups, **opts)
  File "build/bdist.macosx-10.9-x86_64/egg/pymongo/collection.py", line 1164, in ensure_index
    return self.create_index(key_or_list, cache_for, **kwargs)
  File "build/bdist.macosx-10.9-x86_64/egg/pymongo/collection.py", line 1063, in create_index
    **self._get_wc_override())
  File "build/bdist.macosx-10.9-x86_64/egg/pymongo/collection.py", line 412, in insert
    self.uuid_subtype, client)
  File "build/bdist.macosx-10.9-x86_64/egg/pymongo/mongo_client.py", line 1121, in _send_message
    rv = self.__check_response_to_last_error(response, command)
  File "build/bdist.macosx-10.9-x86_64/egg/pymongo/mongo_client.py", line 1063, in __check_response_to_last_error
    raise OperationFailure(details["err"], code, result)
OperationFailure: text search not enabled

----------------------------------------------------------------------

[gitaarik]

@thedrow I don't understand why Travis finds more errors than I do, I only get the one that I posted above, but I also get that one when I run the test on master of the original repo.

[gitaarik]

@thedrow Ok, I never worked with Travis, but I guess it is because of a different environment, will look into it.

[gitaarik]

Ok, now it seems the tests that are failing are not because of my changes, is that correct?

[yograterol]

@gitaarik sync the branch with master please.

[gitaarik]

Ok now this is failing:
https://travis-ci.org/MongoEngine/mongoengine/jobs/29585664#L1285

I'm a bit puzzled about this one. I don't know about this search_text feature, but apparently it sets a field text_score on the document. A bit odd in my opinion, because what if you coincidentally happen to have a field text_score on your document? If would at least be better to add an underscore in front (i.e. _text_score) and then we could add it to the "ignore" list here:

https://github.com/MongoEngine/mongoengine/pull/457/files#diff-e3e316e6686a3db66d2e679634f57f7fR65

Or make a _meta dict where we can put these kinds of meta info about the document. That would allow for more flexibility I think.

[wpjunior]

Append 'text_score' in https://github.com/MongoEngine/mongoengine/pull/457/files#diff-e3e316e6686a3db66d2e679634f57f7fR65.

Solve problem =D

[gitaarik]

@wpjunior Ok, but what do you think of my other suggestions? And what if you have a document with a field text_score? Will it overwrite your field and won't you be able to access this data anymore?

[wpjunior]

I have an idea:

Document.objects.start_search('blah lbh', text_score_field="text_blah")
set automatically the text_score in document by name specified

if field already defined raises an exception: FieldAlreadyExist

[gitaarik]

What do you think of a _meta dict on a document? That would solve any similar problems in the future.

[wpjunior]

@gitaarik see my Mockup:

class MyDoc(Document)
    text_score = FloatField()

    meta = {'text_score_field': 'my_text_score_field'}

[gitaarik]

@wpjunior No, what I mean is the following:

I assume text_score is the score for how well the document matched the text search. So this is information ABOUT the document, that's different from data in fields. Currently you set the text_score on a property directly on the document, just like fields. This could be a bit confusing, but also could make things less flexable. Because the framework now can't be sure if a certain property on a document is a field or not. Also it is a problem in case you happen to have a field called text_score.

That's why I propose to use the _meta property. Metadata is data ABOUT the document, useful information that isn't directly contents of the document itself. I looked in the code and in fact _meta is already a property that exists on a document and is heavily used. For example, look at these:
https://github.com/MongoEngine/mongoengine/blob/master/mongoengine/document.py#L165
https://github.com/MongoEngine/mongoengine/blob/master/mongoengine/document.py#L553

I would propose to also use this _meta property for the text search score. So when you want to add this information to a document, add it to the _meta property. For example:

matched_document._meta['text_search_score'] = text_search_score

[wpjunior]

Other simple alternative is creating a method get_text_score instead of text_score property.

In django not accept {{ mydoc._meta.text_search_score }}, variable starting with _ isn't accepted, it's a problem if you like to show value in django template directly.

[gitaarik]

That solution would probably work, but I'm not sure about the elegance of it. To me, _meta would be the most logical place to store such data. I think the issue you have with Django templates isn't a good reason to make Mongoengine's code less consistent. You can easily work around it by doing something like this:

mydoc.meta = mydoc._meta

But I could be wrong. @rozza Maybe you can say something about what would be the best thing to do here.

[gitaarik]

Ok, it's rebased.

The issue with text_score still exists though. @wpjunior, can you take another look at this? It would be fairly easy to use your text_score feature through the _meta dict on the document object, and it would be a logical place to store such information.

[wpjunior]

I fixed up, removing text score field and creating a method instead.

Very thanks @gitaarik =D

[gordol]

It was merged, just not through the pull request.

https://github.com/MongoEngine/mongoengine/commits?author=gitaarik