Matches method asymmetrical: encounter1.Matches(encounter2) does not imply that encounter2.Matches(encounter1)

The Matches method is asymmetrical, that is, the following can be true: encounter1.Matches(encounter2) && !encounter2.Matches(encounter1). Here is an example of a passing test that exhibits this behavior:

[Test]
public void MatchesAsymmetric_IsExactlySymmetric()
{
  var enc1 = new Encounter {Id = "a"};
  var enc2 = new Encounter();
  
  // Matches return value depends on which direction you call it in
  Assert.IsTrue(enc1.Matches(enc2));
  Assert.IsFalse(enc2.Matches(enc1));

  // IsExactly value is the same in both directions
  Assert.IsFalse(enc1.IsExactly(enc2));
  Assert.IsFalse(enc2.IsExactly(enc1));
}

This seems only to happen when one object has one or more values set which are null in the other object. If each object has a value set which is null in the other, Matches is false in both directions:

[Test]
public void MatchesAsymmetric_IsExactlySymmetric()
{
  var enc1 = new Encounter { Id = "a" };
  var enc2 = new Encounter { Period = new Period(new FhirDateTime(1994), new FhirDateTime(1995)) };

  // Matches is false in both directions: each encounter has a value set which is null in the other
  Assert.IsFalse(enc1.Matches(enc2));
  Assert.IsFalse(enc2.Matches(enc1));

  // IsExactly value is the same in both directions
  Assert.IsFalse(enc1.IsExactly(enc2));
  Assert.IsFalse(enc2.IsExactly(enc1));
}

This behavior is unexpected and difficult to rely on, as it is hard to constantly remember which direction to call the method from in order to ensure consistent behavior.

I believe the following method is the source of the behavior (from Hl7.Fhir.Model.DeepComparable.Matches(IDeepComparable a, IDeepComparable pattern)):

public static bool Matches(IDeepComparable a, IDeepComparable pattern)
{
    if (pattern == null) return true;

    if (a != null && pattern != null)
    {
       return a.Matches(pattern);
    }
    else
        return false;
}

I would think the first line of the method should instead be:

if (pattern == null || a == null) return true;

so that the method returns true for both directions in the above test. This would mean that the Matches call only checks the match for values which are set for both compared objects, and ignores any value which is null in either object.

[ewoutkramer]

Hi @joelmccarthy, the Matches() implements the logic described for "pattern" in the FHIR spec:

"(...) any value in the pattern must be found in the instance. Other additional values may be found too. This is effectively constraint by example. The values of elements present in the pattern must match exactly (case-sensitive, accent-sensitive, etc.)."

This means that if a pattern has as value for an element, then an instance matches that pattern only if it also has that value. Conversely, if a pattern has no value for an element, the instance will match the pattern, regardless of the value of that element.

Hence, pattern matching - as described by the spec - is by definition not symmetrical! It's kind of "match by example" that some ORMs/query languages support: if you don't mention a condition - it matches all, it you do specify a condition - it needs to be matched.

So, yes, if your pattern is null, it matches everything (so XXX.matches(<an empty instance>) is always true), but .matches() will fail. The direction of calling is relevant, the parameter passed in is the pattern. I agree that this is not totally obvious. Maybe IsMatchedBy would be a better name for the function?

Hi @ewoutkramer, I appreciate the explanation. This makes sense, I was misunderstanding the intended use case for this method. I agree that IsMatchedBy would be a better name. It might also be helpful if there were some documentation in comments on the method, perhaps linking here for reference. Let me know though if there is documentation about this somewhere, and I just missed it.

Thanks again!

[ewoutkramer]

No, there is not. We should add it and probably rename the method.

[brianpos]

At least the code comments 😉