Skip to content

Conversation

@elephanter
Copy link
Contributor

#1275 fix with tests

Copy link
Member

@wojcikstefan wojcikstefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @elephanter! Does this PR mean that SomeDoc.objects.only('some_field_thats_not_on_the_doc') doesn't fail anymore?

for field in fields:
try:
field = '.'.join(f.db_field for f in
field = '.'.join(f.db_field if not isinstance(f, six.string_types) else f for f in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could re-write it without a double-negative, i.e. f if isinstance(f, six,string_types) else f.db_field

for subdoc in subclasses:
try:
subfield = '.'.join(f.db_field for f in
subfield = '.'.join(f.db_field if not isinstance(f, six.string_types) else f for f in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@elephanter
Copy link
Contributor Author

elephanter commented May 18, 2017

Problem was in MapField, when trying to get value from it by key. (DictField will work correctly as I know)
_fields_to_dbfields method looking up fields with _lookup_field method, and expected result to be BaseField instance. But _lookup_field returns strings also. For example in case of Array field

            if field_name.isdigit() and isinstance(field, ListField):
                fields.append(field_name)
                continue

I think there was another error, where we requesting some index in array, like

obj.objects().only('arr.0.fld')
and it also was fixed. Maybe we need another test for that case

@wojcikstefan
Copy link
Member

Right, I was just asking if a case that should fail doesn't anymore (which is my hunch). If that's the case, we can't merge this PR.

Just to reiterate, I believe SomeDoc.objects.only('some_field_thats_not_on_the_doc') fails on current master and it still should fail.

Danil Eremeev added 2 commits May 19, 2017 09:42
@elephanter
Copy link
Contributor Author

elephanter commented May 19, 2017

ok. if that test should fail and do not fail now - travis tests must fail. Maybe I don't understand you quite well

@wojcikstefan
Copy link
Member

Hi @elephanter! Sorry for the delay and the confusion. My hunch was wrong and, after a closer inspection of your PR, everything looks good. Thanks again!

@wojcikstefan wojcikstefan merged commit 1d4b187 into MongoEngine:master Jun 19, 2017
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