Report authentication and connection failures as such, not as a parse error

[Natalie-the-technician]

Symptom

Opening a document from a WebDAV server with a wrong password, or from a server
which is down, reports:

Failed to parse document

The file is fine. The user is sent looking for a corrupt project file when the
actual problem is a password or a network.

Cause

ProxyDocument.read() turns every exception into the same message:

​java } catch (Exception e) { throw new DocumentException("Failed to parse document", e); } ​

The cause is preserved, so the information is not lost — it just never reaches
the message the user sees.

Fix

A static helper walks the cause chain and picks a message:

• NotAuthorizedException anywhere in the chain → "Authentication was rejected
  by the server"
• UnknownHostException or ConnectException → "The server could not be
  reached"
• anything else → "Failed to parse document", unchanged

The walk is guarded against an exception which reports itself as its own cause.

Nothing else changes: the same exception type is thrown, with the same cause
attached.

Testing

New ProxyDocumentReadFailureMessageTest: 10 tests. Four cover the new
messages (including a cause nested several levels down and one at the top of the
chain); the rest pin the unchanged behaviour — a genuinely broken file still
says "Failed to parse document", and a self-referencing cause terminates.

Verified the other way round: with the helper returning the generic message
again, exactly those four fail, each with
expected: <Authentication was rejected by the server> but was: <Failed to parse document>
or the connection equivalent. The other six stay green.

A question about wording

The two new strings are hardcoded English, matching the existing
"Failed to parse document" right next to them. If you would rather have them go
through RootLocalizer, say so and I will add the keys — I did not want to
introduce new translation keys without asking.

This is one of four independent WebDAV fixes sent together; the other three are "Do not send a WebDAV document into the local storage pane when saving", "Pass webdav.lockTimeout to the document, and say so when locking is disabled", and "Keep the WebDAV server details visible on a narrow settings page". They touch disjoint sets of files, so they can be reviewed and merged in any order, or individually.

[dbarashev]

Looks good overall. A couple of suggestions though

[dbarashev]

Add SocketTimeoutException, NoRouteToHostException, and SSLException ?

[dbarashev]

i18n these strings?

[Natalie-the-technician]

Both done, thanks. The branch is rebased onto current master and carries three
commits; one of your two line comments shows as outdated because of that.

The three exception types. I did not put all three into "The server could
not be reached", because they do not send the user to the same place:

• NoRouteToHostException joins UnknownHostException and ConnectException
  — same advice, check the address and the network.
• SocketTimeoutException gets its own text, "The server did not answer in
  time". The server may well be reachable and merely slow, and "wait and try
  again" is different advice from "this address does not answer".
• SSLException gets its own text, "The secure connection to the server could
  not be established". You are right that this one is not a reachability
  problem at all: the server did answer, and the user has to look at the
  certificate or the protocol settings, not at the address.

Tell me if you would rather have all three folded into the unreachable-server
message — that is a one-line change either way.

The test for the certificate case deliberately throws SSLHandshakeException,
the subclass an untrusted certificate actually produces, so that matching the
SSLException base class is covered by a real subclass and not only by the
base class itself.

The strings. They now go through RootLocalizer. I looked for existing
keys first: there is no key for "Failed to parse document" — it was only a
literal in ProxyDocument — and no key for a rejected authentication anywhere
in the bundle. The nearest existing family is http.error.*
(timeOut, sslHandshake, unknownHost, …), and I did not reuse it, for
three reasons: it is the namespace of the GPCloud JSON client and nothing else
uses it; its messages embed the raw exception text (Something is wrong with the secure connection: {0}) while these are meant to be plain sentences; and
ProxyDocument also reads local files, where an HTTP namespace would be the
wrong place. Reuse would also not have saved any translation work —
http.error.timeOut and http.error.sslHandshake are present in exactly 1 of
the 46 bundle files, the English one. Say the word and I will switch to
http.error.* instead.

So the five messages use the document.error.read. prefix, which the bundle
already has for document.error.read.unsupportedFormat:

document.error.read.authenticationRejected=Authentication was rejected by the server
document.error.read.insecureConnection=The secure connection to the server could not be established
document.error.read.parseFailure=Failed to parse document
document.error.read.serverUnreachable=The server could not be reached
document.error.read.timedOut=The server did not answer in time

English only — the other languages should come from Crowdin, not from me.
These five lines are not in the branch: they belong in
biz.ganttproject.app.localization, which this PR cannot touch. Until they are
there, every lookup keeps the English sentence as a fallback, the way
Search.kt does for "No results" — otherwise the user would be shown the bare
message key. An empty value falls back too, because the bundle does contain
keys with no value at all. I am happy to drop the fallbacks once the keys are
in the bundle.

On the tests. The ten existing tests still compare the English sentences,
not the keys, so they still describe what the user reads. I checked that they
did not go hollow: replacing the fallback with the key turns 16 of the 18 tests
red, all ten of the original ones among them. Four new tests cover the lookup
itself — a key that is present wins, a missing or empty one leaves the English
text, and the no-argument entry point really does ask RootLocalizer. They use
a second getReadFailureMessage(Throwable, Localizer) overload so that they do
not have to touch global state, and they spell the keys out rather than reading
them from the code, so a renamed key cannot sneak past them.

[dbarashev]

This theoretically can be an infinite loop (if there is a loop in the cause chain). There is Throwables::getCausalChain in Guava that probably deals with such cases.

[dbarashev]

I added the keys from this PR into i18n.properties

[Natalie-the-technician]

Thank you for adding the keys. I checked all five in the bundle before removing
anything: they are in biz.ganttproject.app.localization commit 269851a, which
master already points at, each carrying exactly the sentence it had as a
fallback. The fallbacks are gone, and with them the three tests that covered them.
Dropping them is safe for an error dialog: RootLocalizer.formatText answers a
key it does not know with the key itself, not with an empty string, so a dialog
is never blank.

You are right about the loop, and more right than the PR text was. It claimed the
walk was guarded, but cause == cause.getCause() only catches an exception which
reports itself as its cause. Two exceptions which report each other walk
forever, and so does a loop the chain only enters further down. Measured against
the previous commit: A -> B -> A and A -> B -> C -> B both had to be killed
after 30 seconds, while the self-caused chain returned in one.

I did not take Throwables.getCausalChain, and the reason is where it would be
called from. In guava 31.1-jre, which is what com.google.guava:guava:31.+
resolves to here, it does find the loop, with two pointers of different speed,
but it then throws IllegalArgumentException("Loop in causal chain detected.")
instead of returning a shortened chain. This method runs while a read failure is
being reported, so an exception thrown here would replace the failure the user is
supposed to read about with a failure of the reporting itself. Catching it and
falling back to the generic message would work, but the alternative is shorter
than the try/catch and cannot fail: the walk keeps the exceptions it has already
seen in an identity set and stops at the first repeat. That ends a loop of any
length, and it still visits every exception in the chain, so a transport failure
inside a loop keeps its own message rather than the generic one.

Four tests cover this, each with a timeout on a separate thread, because a
same-thread timeout is only reported once the test method returns and a test
which never ends would take the whole run with it. With the guard taken out
again, exactly the three which walk a loop fail with
TimeoutException: ... timed out after 10 seconds; the fourth, where the
transport failure sits inside the loop, stays green because it is found before
the first repeat.

One more change in the tests: the ones which say which failure gets which message
now hand in a localizer that answers each of the five keys with a marker of its
own, instead of asserting the English sentences. With the sentences in the bundle
those assertions no longer said anything about this code, and the markers also
show that the text comes from the localizer rather than from a constant.