Invariant Rules

[cmoesel]

Invariant rules can be used to:

• access constraint elements not represented with keywords (e.g., requirements)
• access constraint elements that are represented with keywords (if you prefer rule-style assignment)
• add extensions to constraints (e.g., elementdefinition-bestpractice

As part of this, the Description: and Severity: keywords were changed from required to optional since they can now be set by rules. As a result, the rudimentary validation of these also had to move from the importer to the exporter since we can't easily analyze the rules until after RuleSets are expanded.

How it's done

Import is done as normal, allowing for assignment rules, insert rules, and path rules.

When an obeys rule is exported, the StructureDefinition exporter will apply Invariant keywords as normal. It will then iterate the invariant's rules, converting each one to a caret rule expressed in the context of the ElementDefinition's constraints.

For example, given this Invariant and a Profile that uses it:

Invariant: HasGivenName
Description: "The name should include the given name"
Expression: "given.exists()"
Severity: #warning
* requirements = "This is necessary to ensure the patient is addressed properly"
* extension[http://hl7.org/fhir/StructureDefinition/elementdefinition-bestpractice].valueBoolean = true

Profile: MyPatient
Parent: Patient
* name obeys HasGivenName

When the SD exporter processes the rule name obeys HasGivenName, it will apply the Invariant's Description, Expression, and Severity keyword values as normal (creating context[0] on the Patient.name element). It will then convert the Invariant's rules to the following caret assignment rules in terms of the profile:

* name ^constraint[0].requirements = "This is necessary to ensure the patient is addressed properly"
* name ^constraint[0].extension[http://hl7.org/fhir/StructureDefinition/elementdefinition-bestpractice].valueBoolean = true

In this way, we don't have to re-implement all of the complicated assignment logic. Since the Invariant rules are authored outside of the profile, we use the same error logging approach as we do for RuleSets, logging the location of the original rule and where it was applied. E.g:

error The element or path you referenced does not exist: constraint[1].foo
File: /Users/cmoesel/data/fsh/InvariantRules/input/fsh/invariants.fsh
Line: 5
Applied in File: /Users/cmoesel/data/fsh/InvariantRules/input/fsh/patient.fsh
Applied on Line: 11

Open Questions

I'm not sure how we want this to be represented when the user uses the -p flag to get preprocessed FSH. Currently, we don't show the converted caret rules, to the preprocessed profile above looks like this:

Profile: MyPatient
Parent: Patient
Id: MyPatient
* name obeys HasGivenName

When I first implemented this, I didn't create a shallow copy of the rules before inserting the converted caret rules, so the converted caret rules remained in the profile definition and I ended up w/ preprocessed FSH like this:

Profile: MyPatient
Parent: Patient
Id: MyPatient
* name obeys HasGivenName
* name ^constraint[0].requirements = "This is necessary to ensure the patient is addressed properly"
* name ^constraint[0].extension[http://hl7.org/fhir/StructureDefinition/elementdefinition-bestpractice].valueBoolean = true

I thought this may be confusing since HasGivenName has the rules in it -- and saying name obeys HasGivenName doesn't imply keywords-only; so the caret rules look redundant. I didn't think this was what we wanted, but... I wanted to note it in case anyone disagrees.

A third approach, which I haven't implemented, but could be convinced of is to normalize obeys rules out of the definition entirely. In this approach, I'd convert even the keywords to caret rules and then just process them that way. If we did that, then the preprocessed FSH would look like this instead:

Profile: MyPatient
Parent: Patient
Id: MyPatient
* name ^constraint[0].key = "HasGivenName"
* name ^constraint[0].human = "The name should include the given name"
* name ^constraint[0].expression = "given.exists()"
* name ^constraint[0].severity = #warning
* name ^constraint[0].requirements = "This is necessary to ensure the patient is addressed properly"
* name ^constraint[0].extension[http://hl7.org/fhir/StructureDefinition/elementdefinition-bestpractice].valueBoolean = true
* name ^constraint[0].source = Canonical(MyPatient)

I'm interested in your thoughts.

[mint-thompson]

Overall, this looks very good! I think I have found a case that is not covered by the current implementation. It looks like when you define an Invariant that uses rules, and that Invariant is applied to a sliced element, the values from rules don't appear on slices. This is an example of what I mean:

Profile: MyObservation
Parent: Observation
* component ^slicing.discriminator.type = #value
* component ^slicing.discriminator.path = "$this"
* component ^slicing.rules = #open
* component contains Cookie 0..1
* component[Cookie].value[x] only Quantity
* component obeys my-obs-1

Invariant: my-obs-1
Description: "I am asking, please."
* severity = #warning
* extension[http://hl7.org/fhir/StructureDefinition/elementdefinition-bestpractice].valueBoolean = true

In the MyObservation StructureDefinition, severity and extension will not appear on the constraint for Observation.component:Cookie. Only values set by keywords appear on the slice.

Something else I noticed that is probably out of scope for this PR is that the ordering of additional slices and obeys rules can lead to different results. In the above example, if the obeys rules comes first, there is no constraint for Observation.component:Cookie. But, this is existing behavior, and might even be desirable if you want to avoid putting the constraint on the slice (if that's even allowed).

[cmoesel]

Thanks for the review and testing!

when you define an Invariant that uses rules, and that Invariant is applied to a sliced element, the values from rules don't appear on slices

When I saw findConnectedElements used in the applyConstraint function, I meant to check this -- but then forgot. I guess that assignments on caret path don't get propagated to slices the way that constraints do. That's probably good -- because I imagine that in some cases, you would not want to propagate assigned caret values to the slices (for example, propagating slicing to all the slices would be bad).

If we can't propagate caret assignments to slices generally, that means we would need to special case constraint assignments or find some other point in the code to do that propagation. That sounds a little annoying, so I'd like to take a step back and reconsider the whole thing...

Why do we propagate constraints to slices? Is it necessary? Technically constraints on the root should apply to the slices -- and it looks like the validator enforces this. I just created a project with this FSH, containing one profile that propagates constraints and another that does not (thanks to the inconsistency you found). I made instances of each profile with violations of the constraint in slices -- and when I ran the IG Publisher, it flagged the violations whether or not the constraints were propagated to slices.

So... it seems to me that maybe we should stop propagating those constraints at all. This way we don't have to figure out how to propagate some caret rules but not others -- and we also avoid that weird inconsistency where ordering matters. If someone really wants the constraint on the slices too, it's pretty easy to just add more obeys rules...

What do you think?

[mint-thompson]

@cmoesel Since constraints on the base element automatically apply to all slices, I agree there's no need to do any propagation. The output will remain correct, the implementation becomes simpler, and the chance for unexpected results due to rule ordering is removed.

[cmoesel]

OK. 88e1e87 removed the propagation behavior. This PR is ready for re-review.

[mint-thompson]

Hooray for rules on invariants!

[jafeltra]

This makes sense to me. I also agree with your approach for the preprocessed FSH. I find it confusing having the caret value rules without the rules for the keywords. I could be convinced to have your third approach work and include all the rules the invariant will add since it really shows exactly how the invariant is being applied and it does feel like a nice "preprocessed" expansion. That said, I think we can probably hold off implementing that until someone needs/asks for this. Unless you were itching for a reason to do it now.

I did have one question about how we process and include path rules on Invariants. I don't think the path rules are needed beyond setting context, which should already be handled by the importer. That would make Invariants follow a more similar pattern to Profiles/Extensions/etc, which just use path rules for setting context and then toss them out.

[jafeltra]

Are the path rules required to be stay on the rules array of invariants? Previously we had decided that PathRules are only used to set context on all entity types except for Instances. If we are using PathRules to meaningfully set required values on the Invariant, we will need to update the FSH spec.

[cmoesel]

Well... It's complicated? I think I did it this way because Invariant is kind of like Instance, in that it's an instance of a constraint (and only allows assignments and inserts, just like instances). In theory, someone could profile structure definition and slice things, etc -- but now that I think of it, I don't think we need to worry about that because -- (a) I don't think SUSHI would handle that right anyway, and (b) all the properties of ElementDefinition.constraint are 0..1 or 1..1.

So... I think you're right. I think we can toss path rules at the import step. I'll do that when I get a chance.

[cmoesel]

Applied in 593ad1a.

[cmoesel]

@jafeltra and @mint-thompson - this is ready for re-review. Mint, the only thing that changed since your approval is 2cb642d (to toss out path rules).

[jafeltra]

Just one really small comment

[jafeltra]

I don't think you need the boolean for isInstanceRule anymore, right?

Suggested change

	this.visitPathRule(ctx.pathRule(), true);
	this.visitPathRule(ctx.pathRule());

[cmoesel]

Oh. Hmmm... I guess I should check what that boolean actually does, but maybe not.

[jafeltra]

I think it's used for correctly updating soft indexes on path rules, and I think since the rules are tossed out now, the soft index calculations should work like they do for non-Instance entity rules. This is around line 2175 - 2179.

[cmoesel]

You are 100% correct, @jafeltra. In fact, with that set to true, it handles soft indexing on path rules incorrectly. I wrote a test for it and fixed it in e67f989.

[jafeltra]

Thanks! I doubt we even have that test for all the other entity types, so nifty bonus!

[jafeltra]

Looks great! Thanks!