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

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

Closed
chgl opened this issue Feb 23, 2021 · 4 comments · Fixed by #57
Closed

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

chgl opened this issue Feb 23, 2021 · 4 comments · Fixed by #57

Comments

@chgl
Copy link
Contributor

chgl commented Feb 23, 2021

  • fhir.resources version: 6.1.0
  • Python version: 3.9.0 (64-bit)
  • Operating System: Windows 10/Linux

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
Copy link
Owner

nazrulworld commented Feb 23, 2021

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
Copy link
Contributor Author

chgl commented Feb 23, 2021

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
Copy link
Owner

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

@alexsantos
Copy link

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())

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 a pull request may close this issue.

3 participants