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 jsonable_encoder exclude include encode model obj invalid #3078

Closed
wants to merge 1 commit into from
Closed

fix jsonable_encoder exclude include encode model obj invalid #3078

wants to merge 1 commit into from

Conversation

shan-guo
Copy link

Hi, When i use the jsonable_encoder expect covert model to dict obj,use the exclude or include options was invalid

data = jsonable_encoder(queryset, exclude={'id'})

or

data = jsonable_encoder(queryset, include={'name'})

The id always in the data

So i created this pr to solve this problem, Maybe there are better ways to solve this problem

@github-actions
Copy link
Contributor

📝 Docs preview for commit f0adeb7 at: https://6074747ab111dc532ec5a3e7--fastapi.netlify.app

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #3078 (f0adeb7) into master (ce0ec06) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #3078   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          243       243           
  Lines         7419      7427    +8     
=========================================
+ Hits          7419      7427    +8     
Impacted Files Coverage Δ
fastapi/encoders.py 100.00% <ø> (ø)
tests/test_jsonable_encoder.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 2252837...f0adeb7. Read the comment docs.

@xaviml
Copy link
Contributor

xaviml commented May 27, 2021

For someone reviewing this PR: this was raised in #2594, and the open PR for the fix is #2606. There is more than just including the "include" and "exclude".

As of this fix @shan-guo, did you considered nested objects? With your proposed solution, the include and exclude will be recursively passed to the children and they will also be included and excluded, and I am not sure if that is the expected behaviour. For this reason in #2606, I just included "include" and "exclude" to the last "jsonable_encoder". Pydantic solves this recursivity problem by allowing a nested dictionary for "include" and "exclude", but this is not solved either in this PR and in the #2606. It is a different problem to solve.

For the first "include" and "exclude" you passed (BaseModel) is not needed since it is passed to the BaseModel.dict function just above, so it is redundant.

For the second one (dict), it is not needed for what I mentioned before, it will include and exclude children, which I don't think it is exptected.

For the third one (default) is needed since it will be needed for the dict case. This is why this was included in #2606.

Furthermore, the #2606 fixes the problem with include and exclude for dict objects to work like they work in Pydantic BaseModel.

You can see the added tests in #2606 for the use cases.

Regards,
Xavi M.

@shan-guo
Copy link
Author

shan-guo commented May 29, 2021

Sorry i ignore the Pydantic work way ,Thank you answer, I'll close the issue

@shan-guo shan-guo closed this May 29, 2021
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

2 participants