Skip to content

Add config for objectId size #3950

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 5 commits into from
Jun 27, 2017

Conversation

steven-supersolid
Copy link
Contributor

The current objectId length of 10 mixed case alphanumeric characters is only really suitable for classes that contain up to 1 million objects.

This review adds a new constructor option objectIdSize which defaults to the current value of 10, can also be set via the environment variable PARSE_OBJECT_ID_SIZE.

@codecov
Copy link

codecov bot commented Jun 23, 2017

Codecov Report

Merging #3950 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3950      +/-   ##
==========================================
- Coverage   90.59%   90.58%   -0.02%     
==========================================
  Files         115      115              
  Lines        7784     7786       +2     
==========================================
+ Hits         7052     7053       +1     
- Misses        732      733       +1
Impacted Files Coverage Δ
src/defaults.js 90% <ø> (ø) ⬆️
src/cli/definitions/parse-server.js 50% <ø> (ø) ⬆️
src/ParseServer.js 88.6% <ø> (ø) ⬆️
src/cryptoUtils.js 100% <100%> (ø) ⬆️
src/RestWrite.js 93.29% <100%> (ø) ⬆️
src/Config.js 94.92% <100%> (+0.03%) ⬆️
src/StatusHandler.js 99.05% <100%> (ø) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 96.15% <0%> (-0.13%) ⬇️

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 bd6816c...d775992. Read the comment docs.

TylerBrock
TylerBrock previously approved these changes Jun 24, 2017
Copy link
Contributor

@TylerBrock TylerBrock left a comment

Choose a reason for hiding this comment

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

Looks good to me

@TylerBrock
Copy link
Contributor

We might want to add a test to ensure that even when increasing/decreasing the size of an objectId values that are shorter/longer than the current setting still are able to be read and processed without any errors.

@flovilmart
Copy link
Contributor

Let’s make it for 2.6.0, i’d Like to release 2.5.0 ASAP :)

@flovilmart flovilmart added this to the 2.6.0 milestone Jun 25, 2017
@steven-supersolid
Copy link
Contributor Author

Added a test that creates an object with a short objectId then increases the objectId size setting to perform a find, then an update.

The second part of the test creates an object with a long objectId and decreases the objectId size setting to perform a find only. Let me know if you don't think this is sufficient, or overkill as not sure part two is required - increasing the setting should be no different to decreasing, as long as there is no code that checks inequalities. I don't think we actually check anything with the objectId length but could be wrong.

@@ -35,9 +35,8 @@ export function randomString(size: number): string {
}

// Returns a new random alphanumeric string suitable for object ID.
export function newObjectId(): string {
//TODO: increase length to better protect against collisions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Finally someone took care of this //TODO 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's funny, I must have seen that comment over a year ago and it never registered that there might be a problem :)

@natanrolnik
Copy link
Contributor

Waiting release of 2.5.0 to merge this

@natanrolnik natanrolnik merged commit 51d2dd9 into parse-community:master Jun 27, 2017
@Moumouls
Copy link
Member

Hi @steven-supersolid ,
I set up the environment variables "PARSE_OBJECT_ID_SIZE" but apparently this does not change the size of the ID (I have moved to 20). I'm in version 2.6 of Parse Server. Use the environment variables is the good way ?
capture d ecran 2017-10-10 a 15 48 33

Thanks for this add :)
(Sorry for the English, i'm french ) ;)

@steven-supersolid
Copy link
Contributor Author

steven-supersolid commented Oct 10, 2017

Using the environment variable should work if you launch from the CLI but won't work when mounted as express middleware. In that case use the constructor parameter objectIdSize

The setting will also not affect any old id's and I'm unsure how the id's will display in the dashboard, so recommend checking your database entries too.

@Moumouls
Copy link
Member

@steven-supersolid Ok, my Parse server is mounted with express, I will pass the objectIdSize parameter in the constructor, thanks for the quick response! It Works !
capture d ecran 2017-10-10 a 16 28 36
capture d ecran 2017-10-10 a 16 28 06

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