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

Default AUTH_MECHANISM #93

Merged
merged 2 commits into from May 24, 2017

Conversation

Projects
None yet
2 participants
@markunsworth
Contributor

markunsworth commented May 22, 2017

Sets default AUTH_MECHANISM values when using non URI configuration method.

Without this, trying to configure pymongo using a configuration prefix causes a KeyError

(Now with added tests)

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta May 22, 2017

Owner

I don't quite follow why this didn't work with a custom prefix, do you understand that?

Owner

dcrosta commented May 22, 2017

I don't quite follow why this didn't work with a custom prefix, do you understand that?

@markunsworth

This comment has been minimized.

Show comment
Hide comment
@markunsworth

markunsworth May 23, 2017

Contributor

If you specify any auth params(user/pass) it tries to get the
auth_mechanism from the app config, but if you haven't set it then you get
a key error as there is no default.

It's also the case when you don't use a custom prefix (have also added a test for this).

Error
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/mark/development/flask-pymongo/tests/test_config.py", line 171, in test_missing_auth_mechanism_in_nonprefixed_config
    mongo = flask_pymongo.PyMongo(self.app)
  File "/home/mark/development/flask-pymongo/flask_pymongo/__init__.py", line 97, in __init__
    self.init_app(app, config_prefix)
  File "/home/mark/development/flask-pymongo/flask_pymongo/__init__.py", line 279, in init_app
    auth_mechanism = app.config[key('AUTH_MECHANISM')]
KeyError: 'MONGO_AUTH_MECHANISM'
Contributor

markunsworth commented May 23, 2017

If you specify any auth params(user/pass) it tries to get the
auth_mechanism from the app config, but if you haven't set it then you get
a key error as there is no default.

It's also the case when you don't use a custom prefix (have also added a test for this).

Error
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/mark/development/flask-pymongo/tests/test_config.py", line 171, in test_missing_auth_mechanism_in_nonprefixed_config
    mongo = flask_pymongo.PyMongo(self.app)
  File "/home/mark/development/flask-pymongo/flask_pymongo/__init__.py", line 97, in __init__
    self.init_app(app, config_prefix)
  File "/home/mark/development/flask-pymongo/flask_pymongo/__init__.py", line 279, in init_app
    auth_mechanism = app.config[key('AUTH_MECHANISM')]
KeyError: 'MONGO_AUTH_MECHANISM'
assert mongo.db.name == 'test_db', 'wrong dbname: %s' % mongo.db.name
if pymongo.version_tuple[0] > 2:

This comment has been minimized.

@dcrosta

dcrosta May 23, 2017

Owner

Is the else ever going to be hit? I suspect you mean if pymongo.version_tuple[0] == 2: here and L196?

EDIT: oh, right, I need coffee. this is a little confusing so I'd slightly prefer if you would make this ">= 3" to avoid my brain's bugs.

@dcrosta

dcrosta May 23, 2017

Owner

Is the else ever going to be hit? I suspect you mean if pymongo.version_tuple[0] == 2: here and L196?

EDIT: oh, right, I need coffee. this is a little confusing so I'd slightly prefer if you would make this ">= 3" to avoid my brain's bugs.

This comment has been minimized.

@markunsworth

markunsworth May 23, 2017

Contributor

sure - i actually copied an earlier test, so should I update that one too

@markunsworth

markunsworth May 23, 2017

Contributor

sure - i actually copied an earlier test, so should I update that one too

This comment has been minimized.

@dcrosta

dcrosta May 24, 2017

Owner

Ah, so it is -- OK, I don't love that, but won't hold this up any more.

@dcrosta

dcrosta May 24, 2017

Owner

Ah, so it is -- OK, I don't love that, but won't hold this up any more.

@dcrosta

This comment has been minimized.

Show comment
Hide comment
@dcrosta

dcrosta May 24, 2017

Owner

Thanks for the fix here!

Owner

dcrosta commented May 24, 2017

Thanks for the fix here!

@dcrosta dcrosta merged commit 70c7ce1 into dcrosta:master May 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment