fhir.resources icon indicating copy to clipboard operation
fhir.resources copied to clipboard

Dates are not converted when using `construct_fhir_element`

Open ItayGoren opened this issue 4 years ago • 11 comments

  • fhir.resources version: 6.0.0b5
  • Python version: 3.8
  • Operating System: mac

Description

When creating a resource with construct_fhir_element and specifying a date as a string, the date is not converted to Date but rather stay as string. Yet, the regex pattern is verified. I tried multiple resources and it happens for all of them.

What I Did

from fhir.resources import construct_fhir_element
p = construct_fhir_element('Patient', {'birthDate': '2012'})
print(p.birthDate)  # prints `"2012"`
print(type(p.birthDate))  # prints `str`

The same happen when using Patient.parse_obj.

Edit: The following Patient.parse_obj({'birthDate': '2012-01-01'}).birthDate indeed returns datetime.date.

ItayGoren avatar Oct 11 '20 17:10 ItayGoren

@ItayGoren you got right, as python datetime.date requires day, month, year. So when an only year or year+month is provided, the value would always be the exact string.

nazrulworld avatar Oct 11 '20 18:10 nazrulworld

isn't this case in contradiction to the purpose of pydantic? the purpose of it is to force the fields to be of specific type, but in this case I created it with a different type. I have 2 suggestions that I think would be better:

  1. set the default month and day to 1
  2. don't support this case if it's not a valid date

ItayGoren avatar Oct 12 '20 05:10 ItayGoren

Fully agree with you about the contradiction of pydantic , but here we are more strict focused on FHIR specification, thanks to pydantic that gives us the option to handle it in FHIR specification way.

By Default

  1. we should not modify input value.
  2. we cannot do that as according to FHIR Spec, that is valid.

I think currently it is possible to make a custom root validator and achieve your approach, it's depended on project-specific requirements.

But I have got another idea, maybe we can make some option so that it is possible to inject custom validator into Primitive types, that would give a developer a lot of flexibility (like your implement your approach).

nazrulworld avatar Oct 12 '20 08:10 nazrulworld

As a developer, I expect the field to be a date. And that the code datetime.date.today() - patient.birthDate would always work if birthDate is not None. I didn't face it on my own in "real" examples. And yet, I'm afraid of such bugs.

ItayGoren avatar Oct 13 '20 18:10 ItayGoren

I just got such error in production with the following: image My serializer only supports datetime and not date. Hence, I had to manually convert all the dates to datetime

ItayGoren avatar Oct 15 '20 08:10 ItayGoren

I've been thinking about this issue a lot recently. I think what I would like to do is create a customized datetime type that can handle these cases so that the conversion from the FHIR types of Instant, Date, and DateTime all return objects that not only are compatible with the native Python types, but also allow for comparison in ways that make sense while accounting for the flexibility (haha, sparseness might be a better word) of the FHIR spec for these types.

@nazrulworld, what do you think about this? I'm going to start some of the initial work today and let you know where you can check out my progress once there's something to share.

mmabey avatar Apr 08 '21 16:04 mmabey

Okay, so I have a lot of the work done on a new data type for FHIR date/datetime values. One thing I could use some feedback on is what makes the most sense when comparing two objects that have different granularity. For example, would you expect the following to return True or False?

DateTime(2021, 4) < DateTime(2021)  # Does the existence of months in 2021 before April make it less than?
DateTime(2021, 4) > DateTime(2021)  # Does the existence of months in 2021 after April make it greater than?
DateTime(2021, 4) == DateTime(2021)  # Does it make sense to only consider the values we have when testing equality?

What I've been leaning toward is making the first two examples above return True (for the reasons stated in the comments) and the last to return False, since they are not the same value. It's kind of weird for two objects to both be greater than and less than each other but not equal to each other, which is why I'm looking for other perspectives. So @nazrulworld and @ItayGoren, if you have thoughts, please share!

mmabey avatar Apr 14 '21 17:04 mmabey

@mmabey great works! In my opinion, the first and seconds are false and the last one is true. My argument here, as one operand has only a year so we compare with year, not assuming any month (before / after).

We can do some tests here http://hapi.fhir.org/resource . To see how they manage this situation. For example create the same resources with different dateTime, after then search with eq,lt,gt

nazrulworld avatar Apr 14 '21 22:04 nazrulworld

@nazrulworld you're right about the comparisons. I'm not sure why my intuition was off on that, but I confirmed with the HAPI FHIR sandbox that only the populated fields are compared, so the above examples should return False, False, and True like you said.

I've packaged everything up and pushed to GitHub: https://github.com/mmabey/fhirdatetime and I would love your feedback, especially on the the comparison test cases. Next week, I'll work on hooking in some CI/auto testing, getting it up on PyPI, and working on a custom root validator for my own uses. If things go well, @nazrulworld I could see how to hook that work into this project as the default parser for date and datetime values. Let me know if you want me to start in on that last part once you've taken a look at my work.

mmabey avatar Apr 16 '21 23:04 mmabey

First of all, congratulation on the nice work. I will also look at it to see how things work with the default parser. One thing about main class Name, zope foundation already have https://pypi.org/project/DateTime/ class name, ´´DateTime´´ would be nice to rename it.

nazrulworld avatar Apr 18 '21 07:04 nazrulworld

I'm not very concerned with that name collision because DateTime is the name of their project on PyPI, but mine will be fhirdatetime. So with their code you do:

from DateTime import DateTime

And with mine you do:

from fhirdatetime import DateTime

mmabey avatar Apr 19 '21 16:04 mmabey