Skip to content

Conversation

@wojcikstefan
Copy link
Member

@wojcikstefan wojcikstefan commented Dec 6, 2016

Based mostly on landscape.io's report. Increases the health of this package from 75% to 96%. It also increases coverage to 92%.

Some high-level changes:

  • BREAKING CHANGE - renamed ConnectionError to MongoEngineConnectionError since the former is a built-in exception name in Python v3.x. If your existing application catches this exception, you'll need to change your code.
  • BREAKING CHANGE - dropped Python 2.6 support.
  • BREAKING CHANGE - removed import ambiguity - you can no longer do from mongoengine.base import ValidationError. Instead, the preferred from mongoengine.errors import ValidationError is now enforced.
  • Removed mongoengine.base.common.ALLOW_INHERITANCE. This constant was redundant and its removal doesn't change the default inheritance behavior.
  • Moved UPDATE_OPERATORS from mongoengine.base.fields to mongoengine.base.common since they're used in fields and in mongoengine.queryset.transform. Not marking it as a breaking change, because that's an internal constant and shouldn't be imported.
  • Cleaned up the imports and usage of __all__, consolidated use of mongoengine.base in internal submodules vs external modules. None of your existing imports should break.
  • Cleaned up mongoengine.python_support and stopped redefining stuff that's already defined in six. On that note, removed all references to unicode, basestring, etc. from the codebase. What I did not remove is dict.iteritems calls. They are faster and potentially used a lot during runtime. 2to3 takes care of translating them into dict.items in Python v3.x.
  • Removed all the unnecessary sys.path manipulation in tests.
  • Removed outdated tests/migration.
  • Added .landscape.yml to ignore the most common error we're not planning to fix right now (accessing protected vars, e.g. document._meta).
  • Ditched the old except Exception, e syntax.
  • Removed other minor code smells.
  • Documented many of the confusing parts of the codebase.
  • Upgraded coverage tests and fixed coveralls.

Closes #897.

@lafrech
Copy link
Member

lafrech commented Dec 8, 2016

Can you please explain the removal of _import_class calls?

@wojcikstefan
Copy link
Member Author

@lafrech is there a particular place where I dropped an _import_class call that you're concerned about?

@lafrech
Copy link
Member

lafrech commented Dec 8, 2016

@wojcikstefan nope, I misread your comment and thought you were removing it everywhere, so out of curiosity I asked for the explanation. On second thought, I understand you're removing it when it was used for no reason.

Great job, BTW.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a888439 on improve-health-2 into ** on master**.

@wojcikstefan
Copy link
Member Author

Thanks @thedrow! I understand your concern about the size of this PR, but breaking it up into separate PRs, waiting for each Travis to pass, and dealing with conflicts might be an extremely tedious task. I double-checked the diff and will triple-check it right now. I also ran the latest commit against my real-life project's test suite and everything passes. That and this package's 90+% test coverage makes me pretty confident in this PR. Also, I think @gukoff (as humble as he is) did a pretty great job reviewing the changes :)

@wojcikstefan
Copy link
Member Author

Alright, updated the changelog and the upgrade docs, triple-checked the diff, re-ran the tests (both here via travis & locally on mongodb v3.x), and ran a test-suite of my production app utilizing this package. All good to go...

@Kami
Copy link

Kami commented Aug 16, 2019

I know this has been merged a long time ago, but it looks like that with this code change, specifically calling six.text_type(err), our tests fail with latest version of pymongo (3.9.0).

st2.st2common.router: ERROR: Failed to call controller function "post" for operation "st2api.controllers.v1.actions:actions_controller.post": 'ascii' codec can't decode byte 0xc5 in position 129: ordinal not in range(128)
Traceback (most recent call last):
  File "/data/stanley/st2common/st2common/router.py", line 515, in __call__
    resp = func(**kw)
  File "/data/stanley/st2api/st2api/controllers/v1/actions.py", line 129, in post
    action_db = Action.add_or_update(action_model)
  File "/data/stanley/st2common/st2common/persistence/base.py", line 173, in add_or_update
    model_object = cls._get_impl().add_or_update(model_object, validate=True)
  File "/data/stanley/st2common/st2common/models/db/__init__.py", line 451, in add_or_update
    instance.save(validate=validate)
  File "/data/stanley/virtualenv/local/lib/python2.7/site-packages/mongoengine/document.py", line 412, in save
    raise NotUniqueError(message % six.text_type(err))
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc5 in position 129: ordinal not in range(128)

It looks like something has changed in the way unicode is handled. Our test case exercises a path where document key contains unicode data. This test works fine with pymongo < 3.8.0.

I added this comment here and not in pymongo, because I believe this can and should be solved in both places. The actual problem lives in this line https://github.com/MongoEngine/mongoengine/blob/master/mongoengine/document.py#L435

And the root cause is likely this change mongodb/mongo-python-driver@e711408#diff-83f588285a24ab0d1ee8a57f88312a6a.

NOTE: This would only happen if default encoding used by Python is not UTF-8.

@behackett
Copy link

Interesting. That code change was released in PyMongo 3.8, not 3.9. It works around a long standing bug in the server which affects users with non-ascii character encodings. I'm surprised the change is causing this problem since it's an extension of a related change made back in PyMongo 3.3.

We'll have to do more research to sort it out.

@Kami
Copy link

Kami commented Aug 16, 2019

Yes, that's correct, that change was introduced in 3.8 (I should have mentioned in the PR that I also tested it with 3.8).

We can continue in #2147 or I can open a separate issue for it.

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.

8 participants