-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
feat: Add options to disable case insensitivity and force email and username to lower case #8042
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
feat: Add options to disable case insensitivity and force email and username to lower case #8042
Conversation
Thanks for opening this pull request!
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## alpha #8042 +/- ##
==========================================
- Coverage 94.33% 94.20% -0.13%
==========================================
Files 183 182 -1
Lines 14515 13718 -797
==========================================
- Hits 13692 12923 -769
+ Misses 823 795 -28
... and 132 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Can we find something that reads easier than "disable case insensitivity" - avoiding the double negation. It could also be more descriptive - "case sensitivity for what"? |
Currently, case-insensitivity is only used for indexes/queries on email/username fields on User class. I can suggest:
Collation is also the error name returned by the Mongo driver if you try to use "case insensitive" index/query on Mongo Serverless. Using the same wording could help developers to fix the error. I do not have other ideas @mtrezza 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the collation applied for all actions regarding username / email, e.g. also password reset, email verification?
@@ -360,6 +360,15 @@ const relationSchema = { | |||
fields: { relatedId: { type: 'String' }, owningId: { type: 'String' } }, | |||
}; | |||
|
|||
const maybeTransformUsernameAndEmailToLowerCase = (object, className, options) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- remove
maybe
- use one term across all code, currently we have "force" and "transform", let's use "convert" for all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove maybe
but i'll keep transform. It's the term used in this file
update = transformObjectACL(update);
transformUsernameAndEmailToLowerCase(update, className, this.options);
transformAuthData(className, update, schema);
@@ -94,6 +94,12 @@ export interface ParseServerOptions { | |||
databaseOptions: ?DatabaseOptions; | |||
/* Adapter module for the database */ | |||
databaseAdapter: ?Adapter<StorageAdapter>; | |||
/* Disable case insensitivity (collation) on queries and indexes, needed if the you use mongodb serverless | |||
:ENV: PARSE_SERVER_DISABLE_DEFAULT_CASE_INSENSITIVITY */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is it necessary to specify the ENV parameter? The options script should create that automatically from the param name. That also prevents the mistake, that the env var name is different than the param name, as is here.
- Please use correct spelling ("MongoDB") and notation, this should be spelled like full sentences, as it goes into the API docs as prose text.
- Can we find a param name that is easier to read and more descriptive, for example avoiding the double negation, for example
enable...
with a default value oftrue
?
disableCaseInsensitivity: ?boolean; | ||
/* Force username and email to lowercase on create/update/login/signup. On queries client needs to ensure to send lowercase email/username | ||
:ENV: PARSE_SERVER_FORCE_EMAIL_AND_USERNAME_TO_LOWER_CASE */ | ||
forceEmailAndUsernameToLowerCase: ?boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Instead of "force" let call it "convert to".
- Notation as if these were full sentences, this goes into the API docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about ? transformUsernameToLowerCase
and transformEmailToLowerCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
Maybe lets call it |
Yes Do you have a suggestion about "default injection" of |
Let's simply use I don't understand the 2nd part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nice refactoring! Minor suggestions below.
Co-authored-by: dblythy <[email protected]>
Co-authored-by: dblythy <[email protected]>
Real usage feedback: With this PR and options activated I currently succeed in running Parse Server in a fully Serverless env using Google Cloud Run (CPU only reserved during requests) and MongoDB Atlas Serverless and it works pretty well. |
I will reformat the title to use the proper commit message syntax. |
To go ahead and merge this, could you address any open comments? |
Does this resovle issue #8492 ? I just tried to run this PR and still hitting the same error message. |
Real usage feedback: I think by now it is necessary to make Parse Server compatible with serverless environments. |
@mattia1208 As you can see in the conversation this PR has open issues that need to be resolved to get merged. Maybe @Moumouls could review them to see which ones can be closed. And there is a merge conflict to resolve. |
Hi @mattia1208 , happy to here that adjustments that i made allow to run parse server with Azure cosmos DB. It's because Azure Cosmos like Mongo Serverless doesn't support insensitive indexes ? A special not @mattia1208 , Parse Server is quite slow to start form a cold start, what is your serverless setup on Azure to prevent cold start. On my side on GCP Cloud Run i've some minute cron task needed for special events, it helps to keep atleast one instance active wihtout paying for a full hot docker instance. |
Hi @Moumouls , Yes, you're correct. Azure Cosmos, like Mongo Serverless, does not support case insensitive indexes. For my serverless setup on Azure, I use the following: Azure Web App + Database + Redis Cache With this setup, I haven't experienced any cold start issues. I hope this provides clarity! |
I recently came across this PR while searching for a solution for a particular problem. Long story short, AWS’ DocumentDB also suffers from the same issue that this PR is intended to solve. To have this PR merged would be a GIGANTIC help for our team, if we could utilize AWS DocumentDB Elastic Clusters to scale our database horizontally. I tried to install @Moumouls version from this exact branch from this PR and it works absolutely beautifully. Is there any chance that you gentlemen can make this happen in the foreseeable future? Is there anything I can help with to make this happen? Thanks. |
@b10f see #8042 (comment); if you want to help, you could either suggest the changes via PR review (use code suggestion), or use this PR as a start for a new PR and incorporate the changes. |
@Moumouls, any thoughts? |
Hi @mtrezza @b10f @mattia1208 @lilonpro , if someone want to recycle my code and finish last details feel free to continue this one. My team is moving out of Mongo Serverless for some specific reason ( but Mongo Serverless works really well with this PR), if it helps your teams now feel free to finish last details of this PR 🚀 We are close, but i will dedicate some time to parse-server on other PRs like GraphQL. |
@Moumouls, since you are the original author of this PR, and considering that it only needs some merge conflicts to be sorted out, and addressing some review comments, I'm assuming it would not take too much effort. Any chance that you could finish it? Even though you're moving out from Mongo Serverless, this PR would make it possible to use AWS DocumentDB and other providers that still have the same limitation. |
Actually, it's not difficult to finish the PR. I'm a volunteer contributor to Parse Server, and most of the time, I try to send PRs to parse-server when I discover an improvement. However, there is no magic, unfortunately. I work and dedicate my time to parse-server on features that are really used by my team. Now we are moving away from Mongo Serverless, and we do not use DocumentDB, so we are not "motivated" to ship this ASAP 😉. I keep my open-source time for other PRs/projects. Here, the job is almost done. Since we are in an open-source environment, you can easily fork, copy the branch, and provide suggested changes by @mtrezza, and it's done. If it's your first contribution to parse-server, it might take some additional time, but contributing to parse-server is feasible. Even better, you will be able to easily contribute in the future 🚀. I'll be happy to help you start contributing 💯. @b10f |
I have also simplified the comments and removed some of the less important suggested changes (like naming), so it will hopefully be easier to get this PR finished. |
Continued in #8805 |
New Pull Request Checklist
Issue Description
Add options to be able to support MongoDB Serverless (no collation support)
Related issue: #7937
Approach
Add 2 abstract options disableCaseInsensitivity and forceEmailAndUsernameToLowerCase to allow a developer to disable default collations of Parse Server and forceEmailAndUsernameToLowerCase to keep clean usernames and emails in the database.
TODOs before merging