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

🐛 Fix issues introduced by removing SQLAlchemy safeguard in jsonable_encoder #1987

Merged
merged 1 commit into from
Aug 29, 2020

Conversation

tiangolo
Copy link
Member

🐛 Fix issues introduced by removing SQLAlchemy safeguard in jsonable_encoder.

The jsonable_encoder originally had a sqlalchemy_safe=True parameter that would avoid trying to encode any object attribute starting with _sa, as SQLAlchemy creates several attributes with that prefix in the models.

It was there as a quick and early workaround/hack to allow working with SQLAlchemy easily. But I didn't really want to have ORM-specific logic in FastAPI, as FastAPI is independent of ORM (or DB, you could just use MongoDB or ElasticSearch as well).

The main workflow for using any type of ORM with FastAPI is with Pydantic's ORM mode, as described in Tutorial: SQL (Relational) Databases that feature has been there for a long while. So I expected the jsonable_encoder to no longer need that workaround. But I had used the same workaround in e.g. the project generator: https://github.com/tiangolo/full-stack-fastapi-postgresql/blob/master/%7B%7Bcookiecutter.project_slug%7D%7D/backend/app/app/crud/base.py#L35 🤦

So, for now, I'm reverting it, for the cases that depended on it, as noticed by @shawnwall on #1864 (comment)


Later I would like to remove the SQLAlchemy-specific logic.

But I think it would have to be something like discarding arbitrary object attributes that start with _ (but this would include, e.g. a MongoDB ORM object with an _id attribute). Although this would only apply to objects, not dictionaries. And only in the case where it's an arbitrary object, not a Pydantic model.

@codecov
Copy link

codecov bot commented Aug 29, 2020

Codecov Report

Merging #1987 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1987   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          239       239           
  Lines         7079      7079           
=========================================
  Hits          7079      7079           
Impacted Files Coverage Δ
fastapi/encoders.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d7038e...d18f5d1. Read the comment docs.

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

📝 Docs preview for commit d18f5d1 at: https://5f4a4718281ab6b2ed5f5f4a--fastapi.netlify.app

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 this pull request may close these issues.

None yet

1 participant