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

Unique indexes #1971

Merged
merged 32 commits into from
Jun 11, 2016
Merged

Unique indexes #1971

merged 32 commits into from
Jun 11, 2016

Conversation

drew-gross
Copy link
Contributor

@drew-gross drew-gross commented Jun 1, 2016

Huge PR.

Adds a way to create unique indexes, and use this new method for ensuring that usernames and emails are unique. But, some people might already have non-unique emails and username, so if the index build fails, we fall back on the previous method. I think I'll make this trigger 2.3.0 and encourage people to build their unique indexes before upgrading.

For behaviour around nulls: Unique indexes are required to ignore undefined/unset fields for the purposes of uniqueness. This is why we use sparse indexes. For null, the plan is to not allow unique indexes on nullable fields. Since we don't currently have a way to specify that a field is non-nullable, we basically aren't going to expose this interface to users yet.

We already require unique emails, but both null and undefined don't count towards uniqueness, somehow, on Parse.com. I think if anyone actually has null for multiple emails, we'll tell them to make sure to clean up their data before upgrading to 2.3.0.

image

@codecov-io
Copy link

codecov-io commented Jun 1, 2016

Current coverage is 91.98%

Merging #1971 into master will increase coverage by 0.06%

  1. 2 files in src were modified. more
    • Misses +2
    • Hits -2
  2. 2 files (not in diff) in src were modified. more
    • Misses -1
    • Hits +12
@@             master      #1971   diff @@
==========================================
  Files            91         91          
  Lines          6420       6449    +29   
  Methods        1096       1108    +12   
  Messages          0          0          
  Branches       1347       1358    +11   
==========================================
+ Hits           5901       5932    +31   
+ Misses          519        517     -2   
  Partials          0          0          

Powered by Codecov. Last updated by fcd914b...0b0518f

@flovilmart
Copy link
Contributor

@DrewGross if we go for 2.3.0 let's put the upgrade guide in that PR, maybe in a 2.3.0.md and move it to the README.md when releasing?

@drew-gross
Copy link
Contributor Author

Good idea. I was going to put it in the Parse Blog, but having it in the repo sounds like a good idea too.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Jun 2, 2016

@drew-gross updated the pull request.

@drew-gross
Copy link
Contributor Author

At least 2 changes are coming: Don't attempt to create the index every time a user signs up, and don't delete null emails if the index build fails.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

16 similar comments
@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 7, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 8, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 9, 2016

@drew-gross updated the pull request.

@facebook-github-bot
Copy link

@drew-gross updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Jun 9, 2016

@drew-gross updated the pull request.

@ghost
Copy link

ghost commented Jun 9, 2016

@drew-gross updated the pull request.

@skinp
Copy link
Contributor

skinp commented Jun 11, 2016

I just had a look at the new readme file + the new code directly dealing with creating those indexes and it looks good to me. I'd look into why the tests are failing (seems just flaky?) and maybe have someone how knows what they're doing with JS check the rest of the changes.

For the unique index creation part, LGTM.

@drew-gross
Copy link
Contributor Author

One of the other changes in here causes tests to fail if there are any open connections to the server left after the test finishes. This was really helpful for debugging, but a lot of our other tests leave connections open so until we can clean them up, they will be flaky. Fortunately it's easy to detect and fix the flakiness now.

@drew-gross drew-gross merged commit 7e868b2 into parse-community:master Jun 11, 2016
@drew-gross drew-gross deleted the unique-indexes branch June 11, 2016 03:29
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.

5 participants