Enable direct access by default

[mtrezza]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue.

Issue Description

Picking up the experimental feature for direct access to Parse Server within Cloud Code.

• The goal is fix related issues and enable it by default.
• The feature has been used by some in production systems for some time, seemingly without of any major issues.

Direct access has been merged as feature flagged here:
#2316

Related issue: #6637

TODOs before merging

• Add test cases
• Add entry to changelog
• Add deprecation definition
• Add changes to documentation (guides, repository pages, in-code descriptions), remove experimental reference

[dplewis]

Its been a while since I looked into this. If I remember correctly the reason why the tests are failing is because of this the installationId is cloud meaning you won't get back a sessionToken. The check is here.

You should be able to fix the tests by setting an installationId not set to cloud in the test suite.

[mtrezza]

Thanks for the hint! I'll look into this.

[dplewis]

@mtrezza I added a PR #6903 I don't know if it would help with the test cases here.

[MobileVet]

@dplewis @mtrezza It appears to me that the only place directAccess is referenced in the server code is ParseServer.js:188 when it sets up a custom REST controller. As a result, I am having a little trouble understanding exactly what this does.

My guess / hope is that instead of firing an HTTP request for a query made in Cloud Code, it uses the mongo connection to make the request 'directly.' Is that what this option does?

For example, I am hoping that if one had a Cloud Function with 3 queries contained in a Promise.all([]), they would utilize the mongo connection pool (assuming it was larger than 3) and make 3 parallel direct queries versus making 3 queries back to the server endpoint using the REST API.

Is that a correct understanding?
Thanks in advance.

[mtrezza]

@MobileVet Your understanding is correct.

I abandoned the PR because I was removing the option to use direct access entirely, but I am not sure anymore direct access should be the only option. It may be detrimental to load distribution within an target group of server instances. So maybe enable it by default but leave it as an option to disable it.

[MobileVet]

@MobileVet Your understanding is correct.

Wow... this changes everything, lol! I have been using Parse for a long time, but have only recently been delving into optimizations and performance issues. While I understood the REST API was used conceptually, the implications of such knowledge weren't apparent until I was reviewing our Sentry logs the other day.

I abandoned the PR because I was removing the option to use direct access entirely, but I am not sure anymore direct access should be the only option. It may be detrimental to load distribution within an target group of server instances. So maybe enable it by default but leave it as an option to disable it.

At first blush, I can't imagine how this could be slower in our use case. We try to group our queries where ever possible and it is annoying to think they are taking up numerous instances to resolve. Granted, that allows the calling instance to maybe handle some other request while it is asynchronously waiting... but that still feels like more overhead. I suppose if one of those queries is long it could stall the instance and thereby starve other callers... but... hmm. Lots to consider.

@mtrezza Is the only known issue the failing tests due to a session token problem?

[dplewis]

@mtrezza Can you fix the conflict and update from master? A few improvements have been made for directAccess. I want to see how many test fail now.

[mtrezza]

Is the only known issue the failing tests due to a session token problem?

I can't give an answer for which purposes the feature is currently fit. I can say however that the feature seems to be used by some developers in production already.

[mtrezza]

@dplewis Unchanged I’d say. IIRC the issue was the test set-up itself that needs to be adapted for direct access. I’ll give you branch access. I think you already have branch access, if you’d like to look into this.

[MobileVet]

I abandoned the PR because I was removing the option to use direct access entirely, but I am not sure anymore direct access should be the only option. It may be detrimental to load distribution within an target group of server instances. So maybe enable it by default but leave it as an option to disable it.

We are definitely going to give this a shot so I hope that the feature isn't removed entirely. It will be really interesting to see the results in our system. We currently handle ~75k sessions a day which isn't huge, but also nothing to sneeze at.

Thanks for all of your work with it.

[codecov]

Codecov Report

Merging #6636 (723263c) into master (70e1347) will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6636      +/-   ##
==========================================
+ Coverage   93.57%   93.92%   +0.34%     
==========================================
  Files         181      181              
  Lines       13193    13194       +1     
==========================================
+ Hits        12345    12392      +47     
+ Misses        848      802      -46     

Impacted Files	Coverage Δ
src/Options/Definitions.js	100.00% <ø> (ø)
src/Options/index.js	100.00% <ø> (ø)
src/ParseServerRESTController.js	98.50% <ø> (+1.49%)	⬆️
src/Deprecator/Deprecations.js	100.00% <100.00%> (ø)
src/Deprecator/Deprecator.js	100.00% <100.00%> (ø)
...dapters/Storage/Postgres/PostgresStorageAdapter.js	95.52% <0.00%> (+0.15%)	⬆️
src/Adapters/Cache/RedisCacheAdapter.js	87.71% <0.00%> (+75.43%)	⬆️

---

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 70e1347...723263c. Read the comment docs.

[dplewis]

@mtrezza How does this look to you? I also fix a bug.

_CoreManager.default.getRESTController(...).handleError is not a function

[dplewis]

@mtrezza If you are interested in seeing my original solution

https://gist.github.com/dplewis/3e911af29a667a88b9f92fa007e735e2

[mtrezza]

What do you think about keeping the option but default it to true, in case someone prefers to distribute the load through a load balancer?

[dplewis]

I like that idea. If you did that originally all the tests would have passed by setting directAccess: false lol

[mtrezza]

My intention was to run all test with direct access true to make sure everything still works. Now that we keep it optional, do you think it is necessary to run all the tests twice, once with true and once with false?

[mtrezza]

@mtrezza can you update the changelog with a your breaking change suggestions?

@dplewis How do you feel about trying out a phased deprecation for this one? That would mean:

• keeping directAccess: false as default
• adding a deprecation warning that we'll change the default in a future version
• moving directAccess out of experimental status (removing the mentioning of "experimental") --> this would actually also impact the environment key PARSE_SERVER_ENABLE_EXPERIMENTAL_DIRECT_ACCESS, which is an indication that we should not put that word into the key name. So we may change the key now and justify that breaking change with the fact that this was an experimental feature, or actually also add a deprecation warning that this key will be deprecated soon and allow both key variants, PARSE_SERVER_ENABLE_EXPERIMENTAL_DIRECT_ACCESS and PARSE_SERVER_ENABLE_DIRECT_ACCESS.

I would add the implications I described above to the Parse Server options doc for the directAccess parameter. Then, in the future version, we actually add the breaking change to the CHANGELOG, and if we also follow semver, that version would be a major version increment and not earlier than 6 months from now (in the currently suggested versioning system).

[dplewis]

Go for it. Hopefully this will bring awareness to this feature and we can get more feedback.

[mtrezza]

Is that somewhat understandable?

Edit: Typo "executed as a HTTP requests" fixed.

[dplewis]

That is perfect!

[mtrezza]

Note, I'm working on adding the deprecation warning here, so this is still in DRAFT.

[mtrezza]

Once #7303 is merged, the following deprecation definition needs to be added in ./Deprecator/Deprecations.js:

{ optionKey: 'directAccess', changeNewDefault: 'true' }

This will log a deprecation warning on Parse Server launch:

DeprecationWarning: The Parse Server option 'directAccess' default will change to 'true' in a future version.

Then this should be ready for merge.

[mtrezza]

After adding the deprecation definition, Parse Server logs the following warning if directAccess is not explicitly set:

DeprecationWarning: The Parse Server option 'directAccess' default will change to 'true' in a future version.

This is the first attempt to apply the deprecation policy by using the deprecation mechanism to notify developers that directAccess will be enabled by default in a future version. Looking ahead, that breaking change would be introduced in 6.0.0 (2022) after displaying the deprecation warning throughout 5.#.# (rest of 2021).

This PR also adds a "Deprecation" section to the README to list the in-progress deprecations.

[mtrezza]

@davimacedo could you take a look at this failing test?

It fails at line:

parse-server/spec/ParseServer.spec.js

Line 110 in 9563793

expect(stdout).toBeUndefined();

with the expected deprecation warning that this PR outputs:

1) Server Url Checks does not have unhandled promise rejection in the case of load error
3762
  - Expected 'warn: DeprecationWarning: The Parse Server option 'directAccess' default will change to 'true' in a future version.
3763
  ' to be undefined.

Your commit from back then says:

Add test to check inexistence of unhandled promise rejection on server fail

I think it was part of fixing #5573 (comment).

Do you think we can change the test line above to be more specific?

expect(stdout).not.toContain('UnhandledPromiseRejectionWarning');

I just want to make sure I'm not messing up the test here.

[davimacedo]

@mtrezza there is a long time it was merged and it is hard to remember but I agree with your change. It looks good to me.

[mtrezza]

If no further objections, I will merge this later today, since this has already gone through multiple reviews and is stopping other PRs from getting merged. There have only been some minor changes since the last review, so it should be good to go.

[parseplatformorg]

🎉 This change has been released in version 5.0.0-beta.1

[parseplatformorg]

🎉 This change has been released in version 5.0.0