feat: Add options to disable collation and transform email and username to lower case #8767

b10f · 2023-09-28T17:22:28Z

Pull Request

Report security issues confidentially.
Any contribution is under this license.
Link this pull request to an issue.

Issue

Add options to support MongoDB Serverless and AWS DocumentDB which currently does not have collation support.

Closes: #7937

Approach

Add 2 abstract options disableCollation and transformEmailAndUsernameToLowerCase to allow a developer to disable default collations of Parse Server and transformEmailAndUsernameToLowerCase to keep clean usernames and emails in the database.

Tasks

Add tests
Add changes to documentation (guides, repository pages, code comments)
Add security check

parse-github-assistant · 2023-09-28T17:22:31Z

Thanks for opening this pull request!

b10f · 2023-09-29T17:11:53Z

This PR is based on #8042

Original author: @Moumouls

mtrezza · 2023-09-29T20:13:13Z

Failing test is:

1) DatabaseController transformEmailAndUsernameToLowerCase should not find a case insensitive user by username or email with transformEmailAndUsernameToLowerCase
  - Error: This user is not allowed to query email on class _User

b10f · 2023-10-02T19:36:11Z

@mtrezza sorry about that. Fixed.

codecov · 2023-10-02T20:36:02Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (80b987d) 94.29% compared to head (84f94ff) 94.31%.

❗ Current head 84f94ff differs from pull request most recent head c8782d8. Consider uploading reports for the commit c8782d8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #8767      +/-   ##
==========================================
+ Coverage   94.29%   94.31%   +0.02%     
==========================================
  Files         186      186              
  Lines       14785    14794       +9     
==========================================
+ Hits        13941    13953      +12     
+ Misses        844      841       -3

Files	Coverage Δ
src/Options/Definitions.js	`100.00% <ø> (ø)`
src/Options/index.js	`100.00% <ø> (ø)`
src/Controllers/DatabaseController.js	`94.05% <73.33%> (+0.07%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Options/Definitions.js

mtrezza · 2023-10-03T00:15:48Z

src/Options/Definitions.js

@@ -533,6 +539,12 @@ module.exports.ParseServerOptions = {
    help: 'Starts the liveQuery server',
    action: parsers.booleanParser,
  },
+  transformEmailAndUsernameToLowerCase: {


I'm not sure whether it's good to combine this into 1 option. It may be beneficial (for other use cases) to introduce 2 separate options that allow developers to control that for email and username separately. Also because by RFC specification, the email address "[email protected]" is not the same email box as "[email protected]".

@mtrezza I think in this context it only makes sense to disable the 2 together, in case a database engine does not support collation.

These 2 are the only fields with case sensitive indexes, so for the problem we're trying to solve here, it only makes sense to have them as a "package deal".

I understand, but these options will live throughout Parse Server, not just for this specific use case. So this "package deal" may be a "no deal" for other use cases. If we decide to divide them into separate options later on it will be a breaking change and more complicated, so I'd suggest to separate them now. A simple use case may be a DB engine that does support collation but the developer wants all emails to be converted to lower case, but not for usernames, or vice-versa.

Hi @mtrezza @b10f , following YAGNI and KISS, currently there is no a real need to split the option. As @b10f it's really specific to DBs not supporting collation. Here i'm in favor to the bundle, and if someone need separate usage it could be a another PR.

Agree with @Moumouls here. This PR is intended to solve a specific problem that at least 2 database engines that we know of suffer from, and not theoretical problems that may or may not happen in the future.

If we want to speculate on what may happen in the future, in my opinion it is much more likely that these database engines will eventually end up supporting collation, and these options can be removed after.

If the developer wants to convert all emails to lowercase, he already has tools on his hand to do so, that doesn't require a separate Parse Server option setting. That is a use-case specific scenario, and should not be the concern of Parse Server.

Changes need to be considered from a broader perspective, not just for a single use-case. We have seen this over and over again in the past where combined features need to be divided, causing breaking changes which we generally want to avoid. Converting only emails or only usernames to lowercase are valid use cases. The time spent in this conversation is more than would have been needed to split the single option into 2, which takes maybe 5 minutes. If this is more work or more complex then let's merge as a single option, but if it really just takes 5 mins, saving us potential future work for deprecation, etc, then I'd suggest to split this option.

mtrezza · 2023-10-03T23:18:47Z

spec/DatabaseController.spec.js

-describe('DatabaseController', function () {
-  describe('validateQuery', function () {
-    it('should not restructure simple cases of SERVER-13732', done => {
+describe('DatabaseController', () => {


Are these changes related to this PR? If not, please revert. They are all over the PR, please check all changes.

mtrezza

What happens if an existing deployment with existing data with a DB that support collation has the transform email and username option suddenly turned on - is that possible? For example, say a DB has emails in different casings and the developer wants to only store emails in lowercase going forward, can they turn on that option and do a manual data correction for the existing data?

src/Options/Definitions.js

mtrezza · 2023-10-18T20:41:31Z

@b10f Would you want to get this ready for merge? I think this is almost done, I would just separate the two options as suggested above and remove the lint changes.

mattia1208 · 2023-11-03T07:53:18Z

Hello @b10f ,

I hope you're doing well. I wanted to thank you for the great work you've done on this PR. Given its significance and the time that has elapsed, could you find a moment to implement the feedback from @mtrezza ? If you need a hand or if you're pressed for time, please let me know so I can assist with a new PR to complete what you've started.

Many thanks for your dedication!

b10f · 2023-11-03T21:09:52Z

@mattia1208 please do so if you have time, unfortunately I won't be able to get around this in the forseable future

Moumouls · 2023-11-04T18:01:32Z

Hi @mtrezza

What happens if an existing deployment with existing data with a DB that support collation has the transform email and username option suddenly turned on - is that possible? For example, say a DB has emails in different casings and the developer wants to only store emails in lowercase going forward, can they turn on that option and do a manual data correction for the existing data?

If a developer wants to use this PR on an existing server, the developer need to migrate existing data and then activate the option.

mattia1208 · 2023-11-08T12:02:07Z

Hi @mtrezza,
so how shall we procede?
Should we close the current conversations and therefore proceed with the merge or should I still create a new pull request?
Given, as stated by @b10f , that this problem, if solved, would be beneficial to any developer who wants to run parse in a serverless environment. It would be great if this change could be added to the Parse Server 7 alpha in time!

mtrezza · 2023-11-08T14:21:59Z

@mattia1208 see #8767 (comment); if @b10f doesn't have the resources to finalize this PR, then please feel free to open a new PR and split the options, then we can go ahead and merge. Alternatively, @b10f may be willing to give you access to their repository, so you can directly make the changes on this branch, which would be less work.

Sbef98 · 2023-11-08T16:58:32Z

@b10f may we eventually intervene in your repo so that we can fix these conversations without having to create a new pr in case you don't have time to get it done yourself according to @mtrezza requests?
We see both your points, and in my opinion both are valid, but at the same time both are blocking this update that would be considered quite nice by a lot of people for quite small details (or at least they look so compared to the usefulness of this update!)

mtrezza · 2023-11-08T17:05:21Z

Agree; and again, if splitting this option into 2 is a lot of work, then let's merge as is; but if this is really just a 5 mins effort, then these 5 mins will likely save us much more time in the future.

mattia1208 · 2023-11-10T16:05:39Z

@mtrezza new PR #8805

mtrezza · 2023-11-14T00:49:32Z

Continued in #8805

b10f added 2 commits September 28, 2023 19:18

feat: disable collation to support MongoDB serverless and AWS DocumentDB

e408376

test: disable collation to support MongoDB serverless and AWS DocumentDB

a0ea93b

b10f marked this pull request as ready for review September 29, 2023 17:12

b10f mentioned this pull request Sep 29, 2023

feat: Add options to disable case insensitivity and force email and username to lower case #8042

Closed

6 tasks

fix: fix failing tests by using proper query permission

c1a6b53

Merge branch 'alpha' into feat-disable-collation

e94bdc9

mtrezza requested changes Oct 3, 2023

View reviewed changes

src/Options/Definitions.js Show resolved Hide resolved

mtrezza requested changes Oct 3, 2023

View reviewed changes

fix: rerun definitions and docs

ea4310d

mtrezza requested changes Oct 3, 2023

View reviewed changes

mtrezza reviewed Oct 3, 2023

View reviewed changes

src/Options/Definitions.js Show resolved Hide resolved

Merge branch 'alpha' into feat-disable-collation

d4d455e

Merge branch 'alpha' into feat-disable-collation

c8782d8

mattia1208 mentioned this pull request Nov 10, 2023

feat: Add compatibility for MongoDB Atlas Serverless and AWS Amazon DocumentDB with collation options enableCollationCaseComparison, convertEmailToLowercase, convertUsernameToLowercase #8805

Merged

3 tasks

mtrezza closed this Nov 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add options to disable collation and transform email and username to lower case #8767

feat: Add options to disable collation and transform email and username to lower case #8767

b10f commented Sep 28, 2023 •

edited by mtrezza

Loading

parse-github-assistant bot commented Sep 28, 2023 •

edited

Loading

b10f commented Sep 29, 2023

mtrezza commented Sep 29, 2023

b10f commented Oct 2, 2023

codecov bot commented Oct 2, 2023 •

edited

Loading

mtrezza Oct 3, 2023 •

edited

Loading

b10f Oct 3, 2023

mtrezza Oct 3, 2023 •

edited

Loading

Moumouls Nov 4, 2023

b10f Nov 4, 2023

mtrezza Nov 8, 2023 •

edited

Loading

mtrezza Oct 3, 2023 •

edited

Loading

mtrezza left a comment •

edited

Loading

mtrezza commented Oct 18, 2023 •

edited

Loading

mattia1208 commented Nov 3, 2023

b10f commented Nov 3, 2023

Moumouls commented Nov 4, 2023

mattia1208 commented Nov 8, 2023

mtrezza commented Nov 8, 2023 •

edited

Loading

Sbef98 commented Nov 8, 2023

mtrezza commented Nov 8, 2023

mattia1208 commented Nov 10, 2023

mtrezza commented Nov 14, 2023

feat: Add options to disable collation and transform email and username to lower case #8767

feat: Add options to disable collation and transform email and username to lower case #8767

Conversation

b10f commented Sep 28, 2023 • edited by mtrezza Loading

Pull Request

Issue

Approach

Tasks

parse-github-assistant bot commented Sep 28, 2023 • edited Loading

Thanks for opening this pull request!

b10f commented Sep 29, 2023

mtrezza commented Sep 29, 2023

b10f commented Oct 2, 2023

codecov bot commented Oct 2, 2023 • edited Loading

Codecov Report

mtrezza Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

b10f Oct 3, 2023

Choose a reason for hiding this comment

mtrezza Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Moumouls Nov 4, 2023

Choose a reason for hiding this comment

b10f Nov 4, 2023

Choose a reason for hiding this comment

mtrezza Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

mtrezza Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

mtrezza left a comment • edited Loading

Choose a reason for hiding this comment

mtrezza commented Oct 18, 2023 • edited Loading

mattia1208 commented Nov 3, 2023

b10f commented Nov 3, 2023

Moumouls commented Nov 4, 2023

mattia1208 commented Nov 8, 2023

mtrezza commented Nov 8, 2023 • edited Loading

Sbef98 commented Nov 8, 2023

mtrezza commented Nov 8, 2023

mattia1208 commented Nov 10, 2023

mtrezza commented Nov 14, 2023

b10f commented Sep 28, 2023 •

edited by mtrezza

Loading

parse-github-assistant bot commented Sep 28, 2023 •

edited

Loading

codecov bot commented Oct 2, 2023 •

edited

Loading

mtrezza Oct 3, 2023 •

edited

Loading

mtrezza Oct 3, 2023 •

edited

Loading

mtrezza Nov 8, 2023 •

edited

Loading

mtrezza Oct 3, 2023 •

edited

Loading

mtrezza left a comment •

edited

Loading

mtrezza commented Oct 18, 2023 •

edited

Loading

mtrezza commented Nov 8, 2023 •

edited

Loading