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

Support for extensions of primitive datatypes #6

Open
axelv opened this issue Jun 3, 2023 · 5 comments
Open

Support for extensions of primitive datatypes #6

axelv opened this issue Jun 3, 2023 · 5 comments
Assignees

Comments

@axelv
Copy link
Contributor

axelv commented Jun 3, 2023

I had this validation error when parsing the profiles of the mcode IG:

ERROR:__main__:Error parsing mcode/StructureDefinition-mcode-cancer-related-medication-request.json: 1 validation error for StructureDefinition
snapshot -> element -> 18 -> type -> 1 -> _targetProfile

This has something to do with extensions on primitive data types. In this case an extension of targetProfile

Probably, the library is missing support for extensions on primitive datatypes as specified here:
https://build.fhir.org/json.html#primitive

@axelv axelv changed the title Support for underscore prefixed properties that appear in extensions of primitive datatypes Support for extensions of primitive datatypes Jun 4, 2023
ruscoder added a commit that referenced this issue Aug 28, 2024
@ruscoder ruscoder self-assigned this Aug 28, 2024
@ruscoder
Copy link
Member

I've implemented generation for primitive types here #12

It generates _property: Optional_['Element'] for all primitives including union types (_valueBoolean). And the edge case is also covered for _given: Optional_[List_[Element]] when primitive is repeatable.

m0rl added a commit that referenced this issue Aug 28, 2024
Add extensions for primitive types #6
@ruscoder
Copy link
Member

Unfortunately, pydantic treats all fields starting with an underscore as private and does not validate them.

Also, looks like there are no workarounds, because underscore_attrs_are_private and extra=Extra.forbid mutually don't work.

I propose another solution then:

  • Provide Str()/PositiveInt()/Decimal()/Boolean()/... models that will have all attrs from Element and
  • define attributes as union type: e.g. birthDate: Str() | str
  • Adjust constructor to build it from JSON and put extensions into proper wrappers
  • Make Str()/... models serializable into two fields (e.g. birthDate and _birthDate)

I'm not sure that it's implementable, but I'm going to try

@ruscoder ruscoder reopened this Aug 29, 2024
ruscoder added a commit that referenced this issue Aug 29, 2024
@ruscoder
Copy link
Member

ruscoder commented Sep 3, 2024

I rethought the suggested approach and it makes the primitive extension model supreme over the primitive types, it's totally wrong because it forces us to use extensions. Also, the suggested approach has issues with type checking, e.g. p.birthDate has type String | str and it can not be used as str without explicit type conversion str(p.birthDate) or accessing p.birthDate.value. The primitive extensions are rare and they just should be supported. It does not make sense to make the development harder in 99% of cases just because of 1% of primitive extension.

I want to develop the initial idea - since the underscore prefix is forbidden to use, let's use the underscore suffix, e.g. birthDate_. I already saw in the code the similar for class_/etc reserved keywords. It can be easily done using pedantic Field with alias

@ruscoder
Copy link
Member

ruscoder commented Sep 3, 2024

Having _ is not enough because it overlaps with reserved keywords, e.g. import_: str and import_: Element. So, let's adhere to fhir.resources python library where it's exposed as __ext suffix. I can not think about anything better, and this is at least well-known for fhir.resources users

@ruscoder
Copy link
Member

ruscoder commented Sep 3, 2024

Just for the history, earlier I had experiments with inheriting primitive data classes from str, like

class String(str):
    def __new__(cls, value, id=None, extension=None):
       ...

class Patient(BaseModel):
    birthDate: String | str

p = Patient(birthDate=String('value', extension=[...]))

Even though it works and type checked, it adds a lot of mystery behind it, e.g. birthDate is str with id/extension attributes. So, I think we should never forget about Python's Zen which states "Explicit is better than implicit." and don't try to do this "magic".

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

No branches or pull requests

2 participants