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

Reduce queries for supported ManyRelatedField #9211

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

clement-escolano
Copy link

@clement-escolano clement-escolano commented Jan 5, 2024

Description

From the discussion here #8919, a related field with a many=True will only execute one query for all items instead of one for every item.

It currently works for:

  • PrimaryKeyRelatedField
  • SlugRelatedField

There are currently four issues regarding this pull request:

  • if someone is inheriting a field with to_many_internal_value and only override to_internal_value, the overridden value won't be used (because to_many_internal_value will be use instead).
    • We could check if the to_internal_value property was overwritten and raise an error / use the old behaviour in this case but I am not sure this is a good idea
  • For PrimaryKeyRelatedField, when the type is wrong, the type of the first item is raised in the error as it is unclear how to retrieve the problematic item. Some solutions to keep the old behaviour if this is an issue:
    • Check that all items have the same type before querying the database and raise an error if some of them have different type
    • In the exception handling, run the query for every item to check which one has the issue
  • For SlugRelatedField, I add to annotate the related field to detect if an item does not exist and which one. I don't know if this is acceptable or not.
  • The pre-commit check fails for a reason I don't understand

@auvipy
Copy link
Member

auvipy commented Jan 5, 2024

tests seems to be failing after new push, can you recheck please?

@clement-escolano
Copy link
Author

tests seems to be failing after new push, can you recheck please?

It should be good now. There is also some general issues to talk about in the PR description :-)

@clement-escolano
Copy link
Author

@auvipy What do you think about the pull request? Is there some ways I can improve it?

@auvipy
Copy link
Member

auvipy commented Jan 15, 2024

It currently works for:

* PrimaryKeyRelatedField

* SlugRelatedField

There are currently four issues regarding this pull request:

* if someone is inheriting a field with `to_many_internal_value` and only override `to_internal_value`, the overridden value won't be used (because `to_many_internal_value` will be use instead).
  
  * We could check if the `to_internal_value` property was overwritten and raise an error / use the old behaviour in this case but I am not sure this is a good idea

* For `PrimaryKeyRelatedField`, when the type is wrong, the type of the first item is raised in the error as it is unclear how to retrieve the problematic item. Some solutions to keep the old behaviour if this is an issue:
  
  * Check that all items have the same type before querying the database and raise an error if some of them have different type
  * In the exception handling, run the query for every item to check which one has the issue

* For `SlugRelatedField`, I add to annotate the related field to detect if an item does not exist and which one. I don't know if this is acceptable or not.

what is the current standing of the issues now? is this backward compatible? and do you think all other related issues can be resolved in this PR?

@clement-escolano
Copy link
Author

I think all issues should be fixed in this pull request but I am asking for some advice on the issues because I am not sure what is the best way of solving them.
Basically:

  • for the inheritance, do you think checking if the method was overridden is a good idea?
  • for the wrong type, which solutions I am suggesting look the best for you?
  • for the annotate, is this even an issue at all?

@auvipy auvipy requested a review from a team January 16, 2024 09:11
@auvipy auvipy self-requested a review March 7, 2024 08:54
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