Handling errors and bugs

To feed the discussion, I have collected some common error situations in the SDK and discuss how we are handling them (and whether I changed my mind on how we do it).

An excellent source of inspiration can be found at http://joeduffyblog.com/2016/02/07/the-error-model/ (start at "Bugs aren't recoverable Errors!" if you are in a hurry).

public Task<IElementSchema?> GetSchema(Uri schemaUri)
        {
            var canonical = schemaUri.OriginalString;

            if (canonical.StartsWith("http://hl7.org/fhirpath/")) // Compiler magic: stop condition
            {
                return Task.FromResult((IElementSchema?)new ElementSchema(schemaUri));
            }

            var hit = SdStorage.GetByCanonical(canonical, FhirRelease);

            if (hit is null) return Task.FromResult(default(IElementSchema));

            var elementSchemaBytes = SdStorage.GetElementSchema(hit.Id) ??
                throw new InvalidOperationException($"The archive does contain a profile with url {canonical} " +
                $"in FHIR release {FhirRelease}, but the profile has no ElementSchema attached.");

            var result = _elementSchemaSerializer.Deserialize(elementSchemaBytes) as IElementSchema;

            return Task.FromResult(result);
        }

1. Not finding a schema is very common, so common we use "null" as a return value. It's not an exception. But you already know that at some point in the future you want to pass more details back to the caller about the reason of failure. Michel has always wanted to report about why something was not resolvable. And indeed, it was always painful debugging it (duplicate canonicals? Wrong business version?). So a Result might be preferable. Alternatively we could have a general UserException with specific subclasses for these errors and document them. But: the compiler won't help you to make sure you are handling it just like a normal return (as is the case with Return).

2. InvalidOperationException - the archive is corrupt. This is more border line. Is this a developer error? Or should the user try to get a new archive? I decided this was so unrecoverable (there might be many more things broken) that it deserved an abort - so throw an Exception that is not supposed to be caught - except by a catch-all set in place to isolate subsystems - much higher up in the calling stack. I'm not convinced this should even be converted to a validation result (probably the first layer up calling this API) - since the caller of the validator (one more layer up) may just go on with the next validation, which will again fail on such a corrupt archive.

3. Deserialize(). We're calling a 3rd party library that might not cooperate and throw exceptions for any reason. What if the data is corrupt and cannot be deserialized? Again, this would mean a corrupt archive here. Or we changed the class layout so the serializer cannot instantiate the right class or element. In any case, a true exception. So, it's ok to let it propagate up and be treated like 2).

        private static async Task<Assertions> callService(ITerminologyServiceNEW svc, string location, string canonical, string? code = null, string? system = null, string? display = null,
                Coding? coding = null, CodeableConcept? cc = null, bool? abstractAllowed = null)
        {
            var result = Assertions.EMPTY;
            try
            {
                result = await svc.ValidateCode(canonical: canonical, code: code, system: system, display: display,
                                               coding: coding, codeableConcept: cc, @abstract: abstractAllowed).ConfigureAwait(false);

                // add location to IssueAssertions, if there are any.
                return new Assertions(result.Select(r => r is IssueAssertion ia ? new IssueAssertion(ia.IssueNumber, location, ia.Message, ia.Severity) : r).ToList());
            }
            catch (Exception tse)
            {
                result += ResultAssertion.CreateFailure(new IssueAssertion(Issue.TERMINOLOGY_SERVICE_FAILED, location, $"Terminology service failed while validating code '{code}' (system '{system}'): {tse.Message}"));
            }
            return result;

Here, we call a service that might not be available. This is not a bug, and pretty common. The API we are calling will however not return error codes for it but throw. So we use a try to catch (a maybe too general Exception) here, and turn it into a normal result. It's a transient error, so nothing has to notified further. The user can retry later on. We could debate on whether this is still a Result, so we can return either an error OR a validation result. In this case, we have special error assertions that act as the equivalent of Result, but is more meant to report diagnostic information to the end user. Returning Result here would be applicable if we expect upper layers to actually act on this (they cannot interpet an error string). But we don't. Just report to user, so a ResultAssertion is perfectly fine.

public static async Task<Assertions> Validate(Uri uri, ITypedElement input, ValidationContext vc)
        {
            var schema = await vc.ElementSchemaResolver!.GetSchema(uri).ConfigureAwait(false);
            if (schema is null) throw new ArgumentException($"A schema cannot be found for uri {uri.OriginalString}.", nameof(uri));
            return await schema.Validate(input, vc).ConfigureAwait(false);
        }

I think this is wrong. Not having a schema available is not exceptional - the whole point of a resolver is that this can happen. It certainly is not an argumentexception - these are for bugs where the caller of an API has made a mistake. It's not recoverable, the user cannot do anything about it, only the developer can. This is not the case here.

The .NET documentation says:

ArgumentException is thrown when a method is invoked and at least one of the passed arguments does not meet the parameter specification of the called method. The ParamName property identifies the invalid argument. Most commonly, an ArgumentException is thrown by the common language runtime or another class library and indicates developer error. If you throw an ArgumentException from your code, you should ensure that the exception's Message property includes a meaningful error message that describes the invalid argument and the expected range of values for the argument.

So, this is a "normal" situation. The author of the Validation API, nor the developer calling our validation API can be held responsible for this. The user has supplied incorrect inputs or an invalid schema. So, we need to report this back to the user. An ResultAssertion would have been preferable.

  public static CodeSystemBuildResult TryBuildFromCodeSystem(string fileContent, FhirRelease release, out ValueSetBuildOutput? result)
        {
            result = default;
            JsonDocument doc;

            try
            {
                doc = JsonDocument.Parse(fileContent);
            }
            catch
            {
                return CodeSystemBuildResult.Unparseable;
            }

            var re = doc.RootElement;

            var rt = getStringProp(re, "resourceType");
            if (rt != RT_CODESYSTEM) throw new InvalidOperationException($"Expected a CodeSystem, but found a {rt}");
            var systemUrl = getStringProp(re, "url") ?? throw new InvalidOperationException("Found a CodeSystem without a url.");

            // If this CodeSystem is not complete, we're not interested.
            if (getStringProp(re, "content") != "complete") return CodeSystemBuildResult.Incomplete;

            // If this CodeSystem does not define an implicit valueset, we're not interested.
            var valueSetUrl = getStringProp(re, "valueSet");
            if (valueSetUrl is null) return CodeSystemBuildResult.NoImplicitVs;

            var version = getStringProp(re, "version");

            CodeData[]? codes = readConcepts(re, ConceptSource.CodeSystemConcept, defaultSystem: systemUrl);

            result = new ValueSetBuildOutput(new ValueSetData(-1, valueSetUrl, release, version, -1), codes);
            return CodeSystemBuildResult.Built;
        }

This is actually a function returning Result, disguised as a more common TryXXXXX function. But instead of returning true/false, it returns an enum indicating the kind of error found. This might be an alternative to Result (which is unfamiliar to .NET devs), but it works badly with async and the fact that you have to initialize the out variable is always a nuisance. How would the different outcomes have been encoded in a Result? The Result has been designed to be either a result (ValueSetBuildOutput) or an Exception. In this case we probably should have created a new Exception subclass with the CodeSystemBuildResult enum as a property member. In any case, none of the things happen here are exceptional - in fact we are missing a few ARgumentNull exception checks here, which would normally need to be present on public functions.

One category is the streaming errors category:

public void SomeMethod()
{
    XmlReaderSettings xmlsettings = new XmlReaderSettings();
    xmlsettings.Schemas.Add("http://www.company.com/blah", "blah.xsd");
    xmlsettings.ValidationType = ValidationType.Schema;
    xmlsettings.ValidationEventHandler += new ValidationEventHandler(ValidationHandler);
XmlReader reader= XmlReader.Create("somefile.xml", xmlsettings);
while (reader.Read()) { }
}

You can subscribe to an event that is triggered whenever a process encounters an error (here a schema validation error). The parsers in the SDK do this too. Useful for when there is no useful "return value" for the Result, and when you might want to go on processing, collecting errors as you go. Here, too, this validator could still throw an Exception for a true exceptional circumstance (stack overflow, out of memory, invalid arguments to the parser).