Skip to content

Allow creation of indices on default fields #4738

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

Merged
merged 3 commits into from
Apr 25, 2018

Conversation

ClaireNeveu
Copy link
Contributor

This PR resolves #4714. Note that due to #4719 the indices this allows you to create are incorrect as of right now. Once #4719 is resolved the indices created on these fields should be correct (and useful).

CC @TylerBrock

flovilmart
flovilmart previously approved these changes Apr 24, 2018
Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

On principle, I’m Ok with the change. I’ll let @TylerBrock merge as he’s the DB wizard

@flovilmart
Copy link
Contributor

@ClaireNeveu there seems be to an issue with the tests:

https://travis-ci.org/parse-community/parse-server/jobs/370758303#L3202

@ClaireNeveu
Copy link
Contributor Author

The test isues are because it's returning back the _id_ index that it creates by default (which it shouldn't be). I'll look into that.

@flovilmart
Copy link
Contributor

that it creates by default (which it shouldn't be)

If the index is created by default, then it should be there no?

@ClaireNeveu
Copy link
Contributor Author

The index is not listed in all situations. Also if I remember correctly Parse doesn't let you manage this index which makes it more of an implementation detail.

@flovilmart
Copy link
Contributor

@ClaireNeveu, I’m ok updating the test so we don’t really care about the full output but just what matters :)

@codecov
Copy link

codecov bot commented Apr 25, 2018

Codecov Report

Merging #4738 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4738      +/-   ##
==========================================
- Coverage    92.7%   92.67%   -0.04%     
==========================================
  Files         119      119              
  Lines        8582     8584       +2     
==========================================
- Hits         7956     7955       -1     
- Misses        626      629       +3
Impacted Files Coverage Δ
src/Controllers/SchemaController.js 96.48% <100%> (+0.01%) ⬆️
src/Adapters/Cache/InMemoryCache.js 91.66% <0%> (-8.34%) ⬇️
src/RestWrite.js 93.6% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1017ac...a8d35fc. Read the comment docs.

@ClaireNeveu
Copy link
Contributor Author

@flovilmart That's a good call, I've refactored the test so it only cares about the one index.

@ClaireNeveu
Copy link
Contributor Author

Hmm, the failing test seems to be unrelated. It works locally and has nothing to do with the change. It should also be environment-independent since the code its testing has nothing to do with the database backend. The test just seems to be flaky since it relies on timing.

@flovilmart flovilmart closed this Apr 25, 2018
@flovilmart flovilmart reopened this Apr 25, 2018
@flovilmart
Copy link
Contributor

Yep that’s odd. Worst case I’ll merge and investigate the flaky test later.

@TylerBrock
Copy link
Contributor

Looks good, thanks @ClaireNeveu

@TylerBrock TylerBrock merged commit 04588bc into parse-community:master Apr 25, 2018
@TylerBrock TylerBrock deleted the indexDefaultFields branch April 25, 2018 23:07
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* Allow creation of indexes on default fields

* Update test

* Try to address flaky cache test
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.

Cannot create index with default field
3 participants