nazrulworld / fhir.resources

FHIR Resources https://www.hl7.org/fhir/resourcelist.html
https://pypi.org/project/fhir.resources/
Other
372 stars 104 forks source link

Validation error if resource id is set before any mandatory fields #56

Closed chgl closed 3 years ago

chgl commented 3 years ago

Description

It seems that the validation success depends on when resource.id is set - at least that's my interpretation for now. Is this expected behavior?

What I Did

The following crashes with a validation error:

from fhir.resources.observation import Observation

o = Observation.construct()
o.id = "o123"
o.status = "final"

print(o.json())
pydantic.error_wrappers.ValidationError: 1 validation error for Observation
__root__ -> status
  none is not an allowed value (type=type_error.none.not_allowed)

but switching o.id and o.status around works fine:

from fhir.resources.observation import Observation

o = Observation.construct()
o.status = "final"
o.id = "o123"

print(o.json())
{"id": "o123", "status": "final", "resourceType": "Observation"}
nazrulworld commented 3 years ago

First of all thanks a lot for your nice input! Let me explain why this happening:

  1. we are using validate_assignment = True in the BaseModel class's meta config because we want to make sure the right value would be assigned to each field.
  2. So when validate_assignment value is True, pydantic behind the scene calls any available pre/post root validator before it assigns field value (with validation).
  3. We have a pre root validator here https://github.com/nazrulworld/fhir.resources/blob/master/fhir/resources/observation.py#L618 for Observation. This is a special validator for any required element (for example status is required for Observation). NB: we are not using the required option inside field declaration intentionally because of support for primitive data type extensibility. For example, though status is required, but if you use an extension for status, then it becomes optional.
  4. I am afraid that using the construct() might make a broken situation always if more than one fields are required.
  5. Until we get a better solution, we should not use construct() method for all cases.
chgl commented 3 years ago

Thank you for the detailed explanation! The new validation support is incredibly helpful (although the initial migration was a bit painful:). Great work!

Is it OK if I create a PR and update the README.md's "Example: 5:" with a reference to this issue/your comment?

nazrulworld commented 3 years ago

You are always welcome 🙏 to make any PR. Good luck!

alexsantos commented 2 years ago

I have a workaround for this: I use the constructor but with those parameters that are required.

    my_bundle = Bundle.construct(type="message")
    my_bundle.id = str(uuid.uuid4())