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

Dates are not converted when using construct_fhir_element #29

Open
ItayGoren opened this issue Oct 11, 2020 · 11 comments
Open

Dates are not converted when using construct_fhir_element #29

ItayGoren opened this issue Oct 11, 2020 · 11 comments

Comments

@ItayGoren
Copy link
Contributor

ItayGoren commented Oct 11, 2020

  • 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.

@nazrulworld
Copy link
Owner

@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.

@ItayGoren
Copy link
Contributor Author

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

@nazrulworld
Copy link
Owner

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

@ItayGoren
Copy link
Contributor Author

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

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

@mmabey
Copy link
Contributor

mmabey commented Apr 8, 2021

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

mmabey commented Apr 14, 2021

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!

@nazrulworld
Copy link
Owner

@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

@mmabey
Copy link
Contributor

mmabey commented Apr 16, 2021

@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.

@nazrulworld
Copy link
Owner

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.

@mmabey
Copy link
Contributor

mmabey commented Apr 19, 2021

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

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

3 participants