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

Add unique username check #275

Merged
merged 3 commits into from
Apr 26, 2018
Merged

Add unique username check #275

merged 3 commits into from
Apr 26, 2018

Conversation

katyafervent
Copy link
Contributor

No description provided.

@katyafervent katyafervent force-pushed the add-unique-username-check branch 6 times, most recently from bf78c4e to 1a29d94 Compare April 21, 2018 21:01
@naumvd95
Copy link
Contributor

failed flake8
failed tests

>       self.auth_header.destroy()
E       AttributeError: 'dict' object has no attribute 'destroy'```

@katyafervent katyafervent force-pushed the add-unique-username-check branch 13 times, most recently from 6b21898 to 467dd11 Compare April 24, 2018 12:19
@katyafervent katyafervent removed the WIP label Apr 24, 2018
atengler
atengler previously approved these changes Apr 26, 2018
Ekaterina Chernova added 3 commits April 26, 2018 14:23
* Make user username and email unique
* clear database after each test class execution
* So all resources such as database objects wil be cleaned after each test
* Delete objects, created in tests
* Fix modifying env variables (monkeypatching that is used previously
  changes it for the whole session, so it affects other tests)
@atengler atengler merged commit 9fac246 into master Apr 26, 2018
@@ -634,6 +636,14 @@ def validate(self):
if field_object.required and field_object.value is None:
return False, 'Required field {} is None'.format(field)

if field_object.unique and field_object.value:
Copy link

@pigmej pigmej Apr 26, 2018

Choose a reason for hiding this comment

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

from line 639 to 646
Does it really iterate over all objects in the namespace?
I don't see any concurrency checks or sth, so wouldn't it just work not ok in concurrent environments? (I'm pretty sure that it will allow to create objects with unique that is not unique at all.

(I'm not kqueen expert though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The iteration started at 630 line witch is not shown here, validate method is called on every obj save, so if you try to add obj with field that should be unique but not, validation will failed and obj won't save.

Copy link

Choose a reason for hiding this comment

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

If you will create the same key twice and try to save them in the same moment, you have high chances to create it regardless of that check. This operation is not atomic and not thread / process safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to add lock on save operation, will that work?) https://github.com/jplana/python-etcd#locking-module

@ekhomyakova ekhomyakova deleted the add-unique-username-check branch June 26, 2018 09:55
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.

4 participants