`PocoBuilder` implements IExceptionSource, but the source cannot be set

[ewoutkramer]

When a PocoBuilder builds poco's based on ISourceNode or ITypedElement, it forwards the exceptions on these sources to its own ExceptionHandler. This means that PocoBuilder partipicates in the IExceptionSource.ExceptionHandler chain: when this property is not set, it will throw an exception, else it will just raise an even on the IExceptionSource.ExceptionHandler.

However, PocoBuilder is internal and cannot be created by the API user, so this property is effectively never set. This makes it impossible for users to catch errors raised by ISourceNode/ITypedElement (for example, when an invalid date is encountered), and the builder will always abort and throw an exception.

This is of course, not in line with the design, which should enable the users to install their own handlers.

Suggested solution: Add an ExceptionHandler property to the PocoBuilderSettings - the user can then use the ToPoco() overloads (which does actually create the PocoBuilder instance) to set a handler.

Also: what do we want the user to do: if he/she has installed a handler on the source - it would probably trigger twice, once on the source, and once on the PocoBuilder (if we fix this as proposed). Is that desired behaviour? Probably, maybe the user should not install a handler on the source, but just set it using the PocoBuilderSettings.

[brianpos]

When testing out this approach, my handler is called twice. I'll push a branch for this at some stage.

[ewoutkramer]

Not really a bug, but a design error. Still, I'd like to treat it with the urgency of a bug.

[ewoutkramer]

Maybe a bug after all. I've been thinking about it, and the way we found this error, is because of this piece of code (created by @brianpos, and a variant is now in the documentation):

    List<Exception> allExceptions = new List<Exception>();
    var src = FhirXmlNode.Parse("....");

    using (src.Catch((_, arg) => allExceptions.Add(arg.Exception)))
    {
        // navigate through src, or feed the instance to another
        // consumer, which will navigate it for you:
        var poco = src.ToPoco();
    }

I suggested above that you'd explicitly configure the ToPoco(PocoBuilderSettings) to call the handler, but that's a crappy solution. What Brian wrote is really what everyone should be doing, no settings necessary.

The cause of the issue (discovered while writing the documentation for it today ;-)) is the fact that the PocoBuilder catches the errors from the IExceptionSource passed to it, but doesn't re-raise them on the original source. The culprit is here:

https://github.com/FirelyTeam/firely-net-common/blob/develop/src/Hl7.Fhir.Support.Poco/ElementModel/PocoBuilder.cs#L108

The Catch() in this line does not use the forward = true parameter, so the exceptions are only put forward (as Brian has analyzed) to the ExceptionHandler of the PocoBuilder, not the original source. If we change that, the original source node will still raise the error on the original handler (installed with the using).

[ewoutkramer]

By the way, the new documentation page about error handling is here:

http://docs.simplifier.net/fhirnetapi/parsing/errorhandling.html#errorhandling

[mmsmits]

The documentation page has been moved to here: https://docs.fire.ly/projects/Firely-NET-SDK/parsing/errorhandling.html

[marcovisserFurore]

Without breaking changes, we cannot solve this.

[rajithaalurims]

@ewoutkramer Ewout Kramer any idea why below lines of code is throwing an exception instead of adding exceptions to the list ?

ISourceNode source= await FhirJsonNode.ParseAsync(body); using (source.Catch((_, arg) => allExceptions.Add(arg.Exception), true)) { model = source.ToPoco(); }

[ewoutkramer]

Hi @rajithaalurims - reading the thread here I see complaints about stuff being reported twice and not at all. I seem to remember that the error reporting of the ToPoco() is not done correctly, but cannot be fixed without a breaking change. We missed this for the 5.0 release unfortunately.

A better option might be the new System.Text.Json based parsers (and there's Xml support too) documented here: https://docs.fire.ly/projects/Firely-NET-SDK/parsing/system-text-json-deserialization.html

It's a much newer part of the SDK and the error reporting is, I think, exactly what you need, since it will report all errors, instead of stopping halfway. Also, it will return a (partially) parsed result.

See https://docs.fire.ly/projects/Firely-NET-SDK/parsing/system-text-json-deserialization.html