Skip to content

Conversation

@LonelyVikingMichael
Copy link
Contributor

Reference issue:

fastapi-users/fastapi-users#751

This PR aims to allow relationships to be both stored and read on the User model, so that relationships may be retrieved on e.g. GET /me

This is achieved by minor tweaks on the create() and _get_db_user() methods respectively, using only code from ormar itself.

One test has been added, all tests are passing.

Thank you for your work, I really enjoy using FastAPI-Users!

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #1 (7a2a2d3) into main (b2b9fac) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main        #1   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           74        79    +5     
=========================================
+ Hits            74        79    +5     
Impacted Files Coverage Δ
fastapi_users_db_ormar/__init__.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 b2b9fac...7a2a2d3. Read the comment docs.

@frankie567
Copy link
Member

frankie567 commented Oct 13, 2021

Hi @LonelyVikingMichael and thank you for tackling this!

I see that you added a .select_all() statement and it indeed looks like a solution. However, I'm concerned about one thing: let's say that I have objects tied to a user (let's say, blog posts). By doing this, we'll always retrieve every related blog posts each time we get the user, right? While it makes sense for objects like "roles", like you show in the unit test here, it's not for blog posts. Imagine a big application with lot of objects tied to a user (which is often the case), we would be bloated with irrelevant data.

I was thinking about this: we could add an optional select_related class property in __init__ that would be a list of relations we want to retrieve when we get the user. Thus, the end developer could precisely select the objects they want to query (e.g. roles, but not blog posts).

Then, we could do .select_related(self.select_related) instead of .select_all. What do you think about this?

@LonelyVikingMichael
Copy link
Contributor Author

Oh, you are absolutely right.

I like your proposed solution, that should work. I will do a quick test to see what we can pass to select_related() as a default - it would be nice to have it optional.

@LonelyVikingMichael
Copy link
Contributor Author

Hi @frankie567

By doing this, we'll always retrieve every related blog posts each time we get the user, right?

For a moment I thought: simply not adding the relationship fields to the Pydantic models (UserCreate etc) is enough, but this new logic also prevents us from needlessly querying the DB. Thanks for your input

I am just unsure about one thing, should the update() method of OrmarUserDatabase be altered so that relationships may be updated via the User model, or is it better to update those entities directly? There may be unintended side effects in doing so?

@frankie567
Copy link
Member

I guess it would be better if we let the responsibility of saving related objects to the end user. If they really need to save it all at once, they could easily overload the method.

@LonelyVikingMichael
Copy link
Contributor Author

Hey Frankie

Is anything else required to get this approved?

@frankie567
Copy link
Member

Hmm, no, it sounds good! Sorry, it was out of my mind!

@frankie567 frankie567 merged commit 677b064 into fastapi-users:main Jan 3, 2022
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.

2 participants