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

`include` missing from `FHIRAbstractModel.dict()` #90

Closed RafaelWO closed 1 year ago

RafaelWO commented 2 years ago

Additional libraries

Description

I use the pydantic model from fhir.resources as response models in FastAPI endpoints, e.g.

from fhir.resources.patient import Patient
...

@router.get("/patients/{pat_id}", response_model=Patient, response_model_exclude_none=True)
def read_patient_by_id(pat_id: str, client=Depends(fhir_client)):
  ...

What I Did

After upgrading to fhir.resources version 6.2, I get the following error at this endpoint

TypeError: dict() got an unexpected keyword argument 'include'
Click for full stack trace ``` Traceback (most recent call last): File "/path/to/python/env/lib/python3.8/site-packages/uvicorn/protocols/http/httptools_impl.py", line 398, in run_asgi result = await app(self.scope, self.receive, self.send) File "/path/to/python/env/lib/python3.8/site-packages/uvicorn/middleware/proxy_headers.py", line 45, in __call__ return await self.app(scope, receive, send) File "/path/to/python/env/lib/python3.8/site-packages/fastapi/applications.py", line 199, in __call__ await super().__call__(scope, receive, send) File "/path/to/python/env/lib/python3.8/site-packages/starlette/applications.py", line 111, in __call__ await self.middleware_stack(scope, receive, send) File "/path/to/python/env/lib/python3.8/site-packages/starlette/middleware/errors.py", line 181, in __call__ raise exc from None File "/path/to/python/env/lib/python3.8/site-packages/starlette/middleware/errors.py", line 159, in __call__ await self.app(scope, receive, _send) File "/path/to/python/env/lib/python3.8/site-packages/starlette/exceptions.py", line 82, in __call__ raise exc from None File "/path/to/python/env/lib/python3.8/site-packages/starlette/exceptions.py", line 71, in __call__ await self.app(scope, receive, sender) File "/path/to/python/env/lib/python3.8/site-packages/starlette/routing.py", line 566, in __call__ await route.handle(scope, receive, send) File "/path/to/python/env/lib/python3.8/site-packages/starlette/routing.py", line 227, in handle await self.app(scope, receive, send) File "/path/to/python/env/lib/python3.8/site-packages/starlette/routing.py", line 41, in app response = await func(request) File "/path/to/python/env/lib/python3.8/site-packages/fastapi/routing.py", line 209, in app response_data = await serialize_response( File "/path/to/python/env/lib/python3.8/site-packages/fastapi/routing.py", line 127, in serialize_response return jsonable_encoder( File "/path/to/python/env/lib/python3.8/site-packages/fastapi/encoders.py", line 104, in jsonable_encoder jsonable_encoder( File "/path/to/python/env/lib/python3.8/site-packages/fastapi/encoders.py", line 47, in jsonable_encoder obj_dict = obj.dict( TypeError: dict() got an unexpected keyword argument 'include' ```

I guess this is related to the 6.2.0b1 release:

Breaking

  • FHIRAbstractModel.json() and FHIRAbstractModel.dict() parameters signatures are more FHIR specific and additional parameters are removed (pydantic specific).

Relates to #89

nazrulworld commented 2 years ago

@RafaelWO sorry to hear about your problem. Can you please describe your usecase of using 'include'. If it is not conflict with fhir specification, we could our best try to keep this parameter.

RafaelWO commented 2 years ago

I'm not doing this myself, this is part of FastAPI - in this case it is trying to serialize the model to JSON.

Here is the line where the exception raises.

nazrulworld commented 2 years ago

Understand that part. Let's see, I have an idea like this 1.) dict() method could accept any pydantic related arguments with doing no action of those extra arguments. 2.) we could do some warning log instead for those extra arguments.

RafaelWO commented 2 years ago

1.) dict() method could accept any pydantic related arguments with doing no action of those extra arguments. 2.) we could do some warning log instead for those extra arguments.

Sounds good! A warning would be very helpful indeed because it took me a while to figure out what the cause of this error was :monocle_face:

So am I right that it is intended that the resources do not support those pydantic features, e.g. exclude_none, anymore?

nazrulworld commented 2 years ago

by_alias & exclude_none are part of FHIRAbstractModel.dict() and with additional encoder argument are part of FHIRAbstractModel.json()

nazrulworld commented 2 years ago

The new release v6.2.1 should solve this issue.

RafaelWO commented 2 years ago

@nazrulworld sorry to bother you again but I experience a ton of the new warnings in my application:

Observation.dict method accepts only´by_alias´, ´exclude_none´, ´exclude_comments` as parameters since version v6.2.0, any extra parameter is simply ignored. You should not provide any extra argument.
Observation.dict method accepts only´by_alias´, ´exclude_none´, ´exclude_comments` as parameters since version v6.2.0, any extra parameter is simply ignored. You should not provide any extra argument.
...

As outlined above, the reason for this is getting those is using the FHIR models as response_model in a FastAPI endpoint. Within FastAPI, the .dict() method is called with all pydantic args (even if I only use exclude_none - others are False or None) here.

Do you think this can be "fixed" from your side? E.g. issuing the warning only once? Or do you not see this issue being within the scope for this package?

For now my workaround is setting the loglevel to error for the corresponding logger:

logging.getLogger('fhir.resources.fhirabstractmodel').setLevel(logging.ERROR)
nazrulworld commented 2 years ago

I understand the problem, One thing I could do is that instead of a warning log, do debug log. But also can possible to find a way to warn one time. Not sure what will be the best solution, but I am really want to let the user/developer know this message.

RafaelWO commented 2 years ago

Thanks! I would prefer a one-time warning ;)

but I am really want to let the user/developer know this message.

Sure, I understand that! What is the default log level? Maybe logging it as INFO is also an option?

dajaffe commented 1 year ago

Hi @nazrulworld - just came across this in a new FastAPI service I'm working on, using these models as the model responses. To avoid spamming our logs I've had to disable loggers coming from fhir.resources.code. Huge +1 to making the warning a one-time only or downgrading to INFO / DEBUG

blazing-gig commented 1 year ago

Hey @nazrulworld is there any update on this where the warning can be made only once ? Also, I'm not using FastAPI response models. The logs can be seen just by calling model_instance.json(exclude_none=True) explicitly from code. Ideally I don't expect to see any warning at all if explicitly called this way. Let me know your thoughts.

nazrulworld commented 1 year ago

I fully agree with your concerns. I am thinking, maybe is good if we change the log from a warning to debug level.

janwijbrand commented 11 months ago

Re-opening this issue: the change of log level was only applied to the json() call, yet it is also prominent in the dict() call and as a result still "spams" my log. As an alternative approach I proppose to use the warnings API instead. I made a PR https://github.com/nazrulworld/fhir.resources/pull/138 for that. I'm eager to hear what you and other developers using fhir.resources think of this alternative approach.