Project

Profile

Help

Issue #1435

Determine if __cmp__ in Package model is safe with mongoengine

Added by bmbouter almost 6 years ago. Updated over 2 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
High
Assignee:
Sprint/Milestone:
-
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Platform Release:
2.8.0
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Pulp 2
Sprint:
Quarter:

Description

There is a scary todo[0] and some discussion about it during review[1]. This overrides the mongoengine cmp which may have unintentional side effects. For example querysets and caching in mongoengine may rely on the builtin cmp definition behavior for core functionality.

I think the key question to answer here is: "Is overriding the mongoengine the cmp safe?"

Come up with an answer to that question, remove the scary DANGER note, and adjust the code so that it's safe.

[0]: https://github.com/bmbouter/pulp_rpm/blob/2fbaa0221bc9d585067458dd72194ced15078e50/plugins/pulp_rpm/plugins/db/models.py#L171
[1]: https://github.com/pulp/pulp_rpm/pull/741#discussion_r47690165

Associated revisions

Revision bbaddd8f View on GitHub
Added by bmbouter almost 6 years ago

Removes scary TODO about cmp usage on NonMetadataPackage

closes #1435 https://pulp.plan.io/issues/1435

History

#1 Updated by mhrivnak almost 6 years ago

  • Triaged changed from No to Yes

#2 Updated by bmbouter almost 6 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to bmbouter

#3 Updated by bmbouter almost 6 years ago

I've determined this is safe. I'm making a PR removing the scary TODO, but I'm writing what I've learned here.

mongoengine.Document provides __eq__() and __neq__(). It does not provide other rich comparison methods (ie: __lt__(), __ge__(), ...). It also does not provide __cmp__(). Python Data Model docs state that since Python 2.1 "[__cmp__() is called] by comparison operations if rich comparison (see above) is not defined." [0] This behavior ensures that for == and != operations which mongoengine expects to have its Document.__eq__() and Document.__neq__() will be used as expected.

Also since mongoengine doesn't provide rich comparison operators we can be reasonably sure that it's not relying on the mostly non-sense comparison provided by the built in object.__lt__ implementation and other default rich comparison operators. Since mongoengine couldn't make meaningful use of these defaults, Pulp is free to define the comparison with __cmp__() as necessary.

One area of concern comes from the mongoengine's __eq__() and __neq__() being called instead of the __cmp__() when Pulp is comparing the equality or inequality of two units. The __eq__() and __neq__() of mongoengine is an ObjectId comparison of the _id fields. The __cmp__() Pulp has historically used would be a tuple comparison of (epoch, version, release) which could cause things that were considered equal to no longer being equal. In this area I think we should just bugfix as problems come up. If two units are not the same record in the DB, the equality comparison of them should not say they are so this new behavior is more correct.

Note that for Python 3 compatibility we will need to replace __cmp__() usage with the __lt__(), __le__(), __gt__(), __ge__(). When we do that we should not implement __eq__() or __neq__() because we could be affecting mongoengine internal behaviors by doing so.

For fun, here [1] is an interesting example of the motivation for rich operators which uses RPM tracking as the example.

[0]: https://docs.python.org/2/reference/datamodel.html#object.__cmp__
[1]: http://www.gerg.ca/blog/post/2012/python-comparison/

#4 Updated by bmbouter almost 6 years ago

  • Status changed from ASSIGNED to POST

#5 Updated by bmbouter almost 6 years ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#6 Updated by rbarlow almost 6 years ago

  • Status changed from MODIFIED to 5

#7 Updated by dkliban@redhat.com over 5 years ago

  • Status changed from 5 to CLOSED - CURRENTRELEASE

#8 Updated by bmbouter over 2 years ago

  • Tags Pulp 2 added

Please register to edit this issue

Also available in: Atom PDF