feat: Allow custom server connection fail message

[dplewis]

Pull Request

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

Issue

Allows users to change the default connection failed message. Developers will be able to display more helpful messages. This is a breaking change for developers that use Parse._ajax which is unlikely since ajax is used internally.

Closes: #1469

Approach

• Add CONNECTION_FAILED_MESSAGE to CoreManager
• Return Parse.Error in RESTController.ajax
• Update EventuallyQueue API to support custom messages

Tasks

• Add tests

[parse-github-assistant]

Thanks for opening this pull request!

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2446007) 100.00% compared to head (ad464ae) 100.00%.
Report is 1 commits behind head on alpha.

❗ Current head ad464ae differs from pull request most recent head 8e576ee. Consider uploading reports for the commit 8e576ee to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##             alpha     #2011   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           61        61           
  Lines         6137      6137           
  Branches      1497      1497           
=========================================
  Hits          6137      6137           

Files Changed	Coverage Δ
src/CoreManager.js	100.00% <ø> (ø)
src/EventuallyQueue.js	100.00% <100.00%> (ø)
src/ParseObject.js	100.00% <100.00%> (ø)
src/RESTController.js	100.00% <100.00%> (ø)

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

[mtrezza]

This is a breaking change for developers that use Parse._ajax which is unlikely since ajax is used internally.

Internal methods are not considered breaking changes, so we can ignore that fact.

[mtrezza]

This doesn't seem to be the correct fix for the bug. The reported bug is that the error message 'XMLHttpRequest failed: "Unable to connect to the Parse API" "is rather confusing for client users." We do have an existing Parse.Error that has a clear error message The connection to the Parse servers failed., which should be thrown, see:

#1469 (comment)

[dplewis]

This is a feature not a bug fix. I want to change the message entirely as I don't want people to know I use Parse servers or xmlhttprequest under the hood for security reasons. The correct error code (100) is thrown here. If you want you can do.

Parse.CoreManager.set('CONNECTION_FAILED_MESSAGE', 'The connection to the Parse servers failed.');

[mtrezza]

I see, but building a feature on top of incorrect code could make the bug fix more complicated. If the bug is fixed and the error message of Parse.Error 100 is thrown, then the concept of your feature may need to change significantly.

I also think that we may need to think about how we override with custom error messages. The way this is currently designed in this PR seem to require a condition everywhere the error is, but when a Parse.Error is thrown (the bug is fixed), it may make more sense to override the predefined Parse.Error message at it's definition.

[dplewis]

building a feature on top of incorrect code

I'm not sure I'm following you. What is missing from this PR?

[mtrezza]

I think it makes more sense to fix the bug first, and then think about how to override the message of the Parse.Error 100. Overriding the Parse.Error message may require a different solution than this PR currently implements.

[dplewis]

I think it makes more sense to fix the bug first

Basically you want me to throw this by default?

new Parse.Error(Parse.Error.CONNECTION_FAILED, 'The connection to the Parse servers failed.')

[mtrezza]

Yes, apologies for the confusion, I assumed the error messages were hard coded in Parse.Error as well, but I just look it up and they don't seem to be, which is a strange design.

Let's assume we wants to allow customizing other error messages as well in the future. So we would end up with a whole lot of string definitions in the Core Manager. That doesn't seem to be an elegant solution. I wonder if this PR can be rewritten in a way so that it's more flexible. For example instead of creating a separate key for each error message, why not let the developer define the error code and the overriding message? Like so:

[
  {
    code: 100,
    message: "Can't connect to server",
  },
  {
    code: 101,
    message: "Oops.",
  },
]

Then, in the Parse.Error constructor you could simply override the message argument. And that works for all errors, not just error 100, without any additional code.

So I'd change:

1. All default error messages for code 100 to 'The connection to the Parse servers failed.'. (= the bug fix)
2. Add the overriding logic to Parse.Error (= the new overriding feature)

[dplewis]

I think that could work. Let me look into it.

[dplewis]

Parse.Error.CONNECTION_FAILED will have to be a special case handled separately since it could be an xmlhttprequest error or an unparseable response from the server and the developer should be able to see those. All other messages can be customized using the contructor method.

[dplewis]

Closing in favor of #2014