Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the need for adapters to use validation and fhirpath #2893

Open
ewoutkramer opened this issue Oct 9, 2024 · 3 comments
Open

Remove the need for adapters to use validation and fhirpath #2893

ewoutkramer opened this issue Oct 9, 2024 · 3 comments

Comments

@ewoutkramer
Copy link
Member

ewoutkramer commented Oct 9, 2024

Currently, we need PocoElementNode + ScopedNode to get the full functionality needed for validation, resolution and fhirpath. The main reason is that elements cannot refer back to their parents, so things like resolving resources and looking into the context of an element are impossible. Also, it is hard to figure out a location, without knowing your parent.

This has been solved by stateful wrappers like ScopedNode, but these have a few disadvantages:

  • Every time one descends into a hierarchy of elements, new wrappers need to be created,.
  • There is no (reference) equality between these wrappers
  • Wrappers do not implement interfaces of the wrapped pocos
  • Continuous wrapping takes memory and performance.

To remove the need for adapters, we will have to add the notion of a parent (and maybe some additional data like index in a list) to the pocos. There are three ways to do this:

  • Set the parent inside the element setter properties. Unfortunately, this does not work for list items, as .NET's list is not Observable, and we do not know when to set the parent on a new items added to the list
  • Set the parent inside the getter properties. This means that as you navigate down the tree via the properties, we're setting the parent on the returned element, including setting the parent on all items in a list for list properties. This introduces overhead for normal get-only read-only scenario's. Although we do not know how much overhead, it feels like we're adding cycles to a very common access path, to support a less common usecase.
  • Set the parent while navigating down the hierarchy with the new "ITypedElement-like" interface (see discussion below). This means the overhead exists only for users of the ITypedElement interface, but it also means it is possible to fool the system by updating the backing POCO's while also navigating using the ITypedElement interface.

This last bullet's disadvantages can be alleviated by making this navigation explicit and requiring a caset to IScopedNode.

var isn = p.AsScopedNode();
var namex = isn.AsScopedNode().Select("resolve()").First();

Second, we need a replacement for ITypedElement, or adapt ITypedElement to expose the Parent. One could argue we should adapt ITypedElement, but the kind of changes we want to make are deeply breaking:

  • Value should be able to return incorrect values
  • InstanceType should return names of backbones
  • Children() should include Lists (arguably, we do not have to change that, but the mental model of Json and POCOs include the notion of lists, rather than repeating elements)
  • Do we really want to have "Name" still there, as this is very much incompatible with how POCOs are structured. A: Yes, we need it for the location anyway.

PS: We've for now chosen to stick to IScopedNode, a subclass of ITypedElement that exposes a Parent property. Once we got that change into our codebase, we can think about introducing lists (basically moving from the XML world with repeated elements to the Json world with lists as first class citizens). We have already seen that Definition is hardly necessary anymore (at least for our goals), so the POCO's will not implement that part of ITypedElement. If you need that, use the old stack.

@brianpos
Copy link
Collaborator

brianpos commented Oct 9, 2024

I don't want this change to make it so that things like the XmlNode or JsonNode which are the random content parsed able to be used with the fhirpath engine (without a known class model), and potentially the validator (I don't do this at the moment - but would for the randon logical models - which don't use reflection).

But I expect the plan is to use the overflow area right?

@ewoutkramer
Copy link
Member Author

ewoutkramer commented Oct 17, 2024

But I expect the plan is to use the overflow area right?

That's exactly it. We should be able to turn any random ITypedElement or even ISourceNode into a DynamicResource with overflow, and then use it everywhere we expect a POCO/IScopedNode.

@ewoutkramer
Copy link
Member Author

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants