FirelyTeam / firely-net-sdk

The official Firely .NET SDK for HL7 FHIR
Other
829 stars 345 forks source link

Improved resolver configuration for Validator & SnapshotGenerator #858

Closed wmrutten closed 1 year ago

wmrutten commented 5 years ago

When the Validator class needs to expand a Snapshot component, it creates an internal SnapshotGenerator instance. Currently, the validator injects it's own ResourceResolver instance into the SnapshotGenerator. The `ValidationSettings.ResourceResolver' property allows consumers to inject a custom resolver.

However, some consumers (such as Clovis) need to specify a different resolver for the internal snapshot generator. Currently, this is not possible.

Proposal:

By default, behavior will be the same as now; the SnapshotGenerator class will recurse on it's own resolver, using internal annotations to detect already expanded snapshots and prevent redundant re-generation. This depends on annotations, which don't survive serialization. However, systems that cannot rely on annotations (e.g. because of serialization) can inject their own resolver with custom logic to determine if existing snapshots should be re-used or re-generated.

ewoutkramer commented 5 years ago

I don't think we should do this - it puts even more logic about snapshot building into the validator - while it should have less. I agree that my initial design is too "user friendly" in the sense that it has flexibility and multiple ways to handle a missing snapshot.

In fact, I think the validator should give all that responsibility to the IResourceResolver, which would then be responsible for creating a snapshot (Michel already created this resolver, but I have not changed the validator).

The flexibility that we currently have also has the advantage that we CAN disable all "smartness" of the validator, by setting the GenerateSnapshot setting to false. No longer will the validator try to create its own Snapshot generator and no longer will the Validator then be involved with snapshot generation. This is probably the "best practice" - just be aware you'd need to add a SnapshotGeneratorResolver (don't remember the name exactly anymore) to the chain of IResourceResolvers passed to the validator.

wmrutten commented 5 years ago

Indeed, the alternative approach would provide better separation of concerns.

Do you think that would work?

ewoutkramer commented 5 years ago

Yes, that would work.

Or...find out whether the IResourceResolver passed to the validator contains a SnapshotSource in the chain (maybe using the Annotations) - if not add it. That way most users don't have to worry about adding a snapshotsource, while IF you add your own one, we'll use that one. Without the need for extra configuration options.

wmrutten commented 5 years ago

Reliably detecting the behavior of an injected resolver is an open question (TODO)... Yes, in theory, API logic could try and navigate the resolver parent hierarchy and detect "well-known" types, but that's merely a hack; e.g. we wouldn't detect a custom ClovisSnapshotSource implementation, unless derived from SnapshotSource. Interesting architectural design challenge btw... Annotations might just do the trick. Wrapping resolvers would pass-through annotations from internal child resolvers, w/o having to understand them. Consumers could get access to all annotations, provided they know what they are looking for. Then publish a list of well-known default resolver capability annotations from the API.

Note: I've sometimes wanted to detect if a resolver in the chain is idempotent (caching), i.e. if a call to Resolve for a specific uri always returns the same object instance (so Annotations survive). I try to traverse the resolver chain to detect CachedResolver, but that's a hack.

ewoutkramer commented 1 year ago

We've mostly done this now - the source passed to the validator indeed has a snapshot resolver. The old validator can still run its own, the new validator doesn't know anything at all about snapshots, it's all handled externally.