Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue warning when map_entry can't find field for query #183

Closed
wants to merge 1 commit into from

Conversation

lafrech
Copy link
Collaborator

@lafrech lafrech commented Mar 19, 2019

While working on map_query, I realized a shortcoming of the field/name mapping in the inheritance case.

In the framework tests, we use the parent class to find a child element based on a query on a field that does not exist in the parent class.

InheritanceSearchParent.find_one({'sc1f': 1})

It works, but I believe it is wrong. The query mapper does not have access to the field, so it can not do its job. In fact the query will fail if the field is an EmbeddedField (I could reproduce that) because in map_entry we won't pass in the dedicated case as field is None:

elif isinstance(field, EmbeddedField):

I don't think we can pass the child classes fields here, and it would be a bad idea because different children could have different fields for the same name, so the field is undefined.

Since it works in some (in fact, most) cases anyway, I propose to just add a non-breaking warning at least for now. But I can make this an Exception. Or if we plan to forbid this in the next major version, I can use a DeprecationWarning.

Or do we want users to be able to pass query entries that don't match a field known to the model?

This is a corner case, but since the wrong use was shown in the tests, users may be relying on it.

I can review the warning/exception type, the message string (add a note about inheritance ?), and perhaps rework the framework tests to isolate this specific test.

Feedback welcome.

@lafrech lafrech requested a review from touilleMan March 19, 2019 17:19
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 96.18% when pulling a719444 on warning_map_entry_field_none into 0424772 on master.

@lafrech
Copy link
Collaborator Author

lafrech commented Mar 20, 2019

This raises a warning in at least one legit case:

    {'$near': {'$geometry': {'type': 'Point', 'coordinates': [122.0, 31.0]}}}

Maybe we should be totally loose and not try to warn the user at all.

I still think the framework tests should be reworked so as not to expose a use case that can't really be supported.

@lafrech lafrech added this to the 3.0 milestone Oct 20, 2020
@lafrech lafrech closed this in #320 Dec 2, 2020
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.

None yet

2 participants