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

Allow the model to store invalid data and give detailed exceptions about it #3079

Open
wants to merge 14 commits into
base: develop-6.0
Choose a base branch
from

Conversation

Kasdejong
Copy link
Member

@Kasdejong Kasdejong commented Mar 13, 2025

Description

  • Allows the model to store invalid data in the overflow and report errors on it when the matching getter is called
  • Also added exceptions for when invalid data is found in the corresponding overflow entry when the property is being requested

Related issues

implements #3056

Codegen PR: FirelyTeam/fhir-codegen#55

# Conflicts:
#	src/Hl7.Fhir.Base/Model/Generated/Address.cs
#	src/Hl7.Fhir.Base/Model/Generated/HumanName.cs
#	src/Hl7.Fhir.Base/Model/Generated/Ratio.cs
@Kasdejong Kasdejong marked this pull request as ready for review March 13, 2025 10:09
@Kasdejong Kasdejong marked this pull request as draft March 13, 2025 10:09
@Kasdejong Kasdejong marked this pull request as ready for review March 13, 2025 15:31
@ewoutkramer
Copy link
Member

      /// <summary>
      /// Defines how the part works
      /// </summary>
      /// <remarks>This uses the native .NET datatype, rather than the FHIR equivalent</remarks>
      [IgnoreDataMember]
      public string? Definition
      {
        get => _DefinitionElement?.Value;
        set
        {
          DefinitionElement = value is null ? null : new Hl7.Fhir.Model.Canonical(value);
          OnPropertyChanged("Definition");
        }
      }

The helper properties are exposing the null markers, and won't trigger an exception. Maybe let the getters just use the normal property instead of the backing field, that should solve the problem

@ewoutkramer
Copy link
Member

We've hit a subtle snag here, which is characteristic of everything we've with moving over to the POCOs: Since POCO is King, then so is the model, defined by the attributes on the POCOs. However, we don't always stick to it for now - mostly because of the "Since" parameters on the attributes. This means that for SOME versions of FHIR a datatype is different, or an element did not yet exist. We're not using this information at all in the new model validation. We did in the parsers (they are using the right version of the ModelInspector), but our (attribute) validators don't account for it yet. Also, when we return the type of the element in the PocoNode, we return the physical type, which is not always the FHIR type (this is, I think, only relevant for the ofType in FhirPath).

The good news is: we have the information. Since we're dealing with POCOs and this is encoded on the POCOs. However, we've shied away from using metadata most of the times (although we annotated ModelInspector sometimes I think), and the ValidationContext has no notion of FHIR version yet.

So, I have moved #2982 on this sprint, since I think we should do something with the FHIR version while validating the model:

  • While parsing, this is simple, the parser has the ModelInpector, and can pass it to the model validation
  • While calling "attribute validation" outside of the parsers, we'lll probably have to pass in the ModelInspector to make the attributes/IValidateObject sensitive to FHIR version.

A nice last design discussion!

@Kasdejong Kasdejong marked this pull request as draft March 18, 2025 11:14
@Kasdejong Kasdejong requested a review from ewoutkramer March 20, 2025 12:55
@Kasdejong Kasdejong marked this pull request as ready for review March 20, 2025 12:55
@Kasdejong Kasdejong changed the title DRAFT: implement model errors using overflow Allow the model to store invalid data and give detailed exceptions about it Mar 20, 2025
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

Successfully merging this pull request may close these issues.

2 participants