Handle Backup Server severity messages properly. Fixes #69.

[chgeuer]

Is it OK to simply issue a pull request here?

[senseibaka]

Yep, certainly OK to raise a PR (congrats on being the first external PR!)

I have some questions:

• How does the reference (SAP) driver handle this scenario? i.e. Does it treat backup server messages as errors if the inline severity value is > 1?
• What is the value of the EED Token's Severity field when backup server emits errors? how about when it emits informational messages?
• Follow-up to that is: Can you provide unit tests around this? It'd be nice to have EedToken instances constructed with life-like data, and also to prevent regressions.

Other notes:

• I think we should avoid using Regex -- in particular my concern/worry is that we include too many namespaces and (negatively) affect performance. This is in part why we try to avoid using Linq, as an example.
• int.Parse calls need to be done in a safe manner

Cheers

[chgeuer]

Answer the questions first:

• I unfortunately have no idea what the "reference (SAP) driver" is. In my other implementation (Python-based) I shell out to the isql executable and parse text directly. So cannot answer that one.
• The EED's token has Severity=2 in cases where the Backup Server returns a hard error. (see the unit test data which I added).
• Unit test - Done.
• Getting rid of RegEx: Done... Sorry for being lazy in the first shot.
• Getting rid of insecure int.Parse(): Done. Again, sorry for being lazy in the first shot.

Below is mock data which came from an ASE16, when I run a SQL dump command to a non-existent directory

dump database AZU to '/doesnotexist/foo' with compression = '101'

List<Tuple<int, string>> aseSeverityAndMessage = new List<Tuple<int, string>>()
{
    // informational from backup server
    new Tuple<int, string>(1, "Backup Server: 4.172.1.4: The value of 'allocated pages threshold' has been set to 40%."),
    new Tuple<int, string>(1, "Backup Server session id is: 18. Use this value when executing the 'sp_volchanged' system stored procedure after fulfilling any volume change request from the Backup Server.\n"),
    // informational from backup server
    new Tuple<int, string>(1, "Backup Server: 4.41.1.1: Creating new disk file /doesnotexist/foo."),
    // 
    // error from backup server (severity ASE == 2) 4.141.2.40 means 4="I/O service layer error", 2="unexpected condition, possibly fatal to the session, has occurred"
    // 
    new Tuple<int, string>(2, "Backup Server: 4.141.2.40: [11] The 'open' call failed for database/archive device while working on stripe device '/doesnotexist/foo' with error number 2 (No such file or directory). Refer to your operating system documentation for further details.\0"),
    new Tuple<int, string>(16, "Error encountered by Backup Server.  Please refer to Backup Server messages for details.\n")
};

[senseibaka]

Ok, so the final message's Severity is 16. This will trigger an AseException, which is good. From that point of view, the driver's existing behaviour is working as-expected.

I did some investigation into how the reference driver behaves, and it reports the final (severity 16) message as the Error message, but also includes the prior informational messages in the AseException. I reckon this is what actually needs to be replicated.

So, what that means:

• The MessageTokenHandler should store all received messages.
• If there's a Severity > 10 message, report all the received messages via the AseException(params AseError[] errors) constructor.

[chgeuer]

This means instead of only collecting t.Severity > 10 tokens in _errorTokens, it should collect all tokens, and raise an AseError with all tokens, if one of the tokens is an error. Is that understanding correct?

Question: When t.Severity is not greater than 10, but the underlying message indicates a Backup Server error, should it raise then, too? In my opinion, the answer is yes..

Do you want me to update the pull request's code?

Additional question: When this pull request is integrated, how long do you expect it to take until available in the regular NuGet feed?

[senseibaka]

This means instead of only collecting t.Severity > 10 tokens in _errorTokens, it should collect all tokens, and raise an AseError with all tokens, if one of the tokens is an error. Is that understanding correct?

Correct

Question: When t.Severity is not greater than 10, but the underlying message indicates a Backup Server error, should it raise then, too? In my opinion, the answer is yes..

In this case, the answer is... practically no (but technically unknown/maybe). This is because, as investigation reveals, the server follows-up the backup server messages with its own error message. The great thing is, this means that the token handler can remain relatively simple, which helps in everyday performance and risk reduction.

Do you want me to update the pull request's code?

Go for it ;)

[chgeuer]

One additional Q: The MessageTokenHandler.AssertNoErrors()currently shuffles ("sorts") the errors in descending order of severity. When that collection now also contains informational messages, that sort operation basically obfuscates the time-wise order of messages.

How about having the AseException.Errors collection remain in the original order (as send from server), and have the .Message property refer to the most severe one?

[senseibaka]

Fair point.

The solution sounds good, I like it.

[chgeuer]

I'm not sure what CodeFactor identifies here. When I try to access the details, I get a "permission denied".

By the way, Nicolas, if you like, we can have a quick call via Skype for Business. My IM is "$(my github alias)@microsoft.com".

[senseibaka]

CodeFactor found an issue: Insert parentheses within the conditional AND and OR expressions to declare the operator precedence.

It's currently on: src\AdoNetCore.AseClient\Internal\Handler\MessageTokenHandler.cs:27 Commit 5740cf6649cd1ce1d38f6366a8acbcbe262e20c7

---

I can't find any settings to make CodeFactor's results publicly visible -- did you try logging in?