[Django] #36369: Missing test coverage for FORWARD_PROPERTIES entries

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Normal
Keywords: FORWARD_PROPERTIES, | Triage Stage:
Options | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
possibly also with REVERSE_PROPERTIES

With the exception of `_forward_fields_map` you can remove entries of
field-related properties from `FORWARD_PROPERTIES` and see no test
failures. Discovered during #36207
[https://github.com/django/django/pull/19245#discussion_r2062359974
review]
--
Ticket URL: <https://code.djangoproject.com/ticket/36369>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------

Reporter: Clifford Gama | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* stage: Unreviewed => Accepted

Comment:

I think this is worth covering, because at a glance it seems like a bug
that `db_returning_fields` is omitted from the entries. A test would help
clarify.
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:1>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Senthil Kumar):

I'm new and would like to take this on. May I assign it to myself?
Assuming it's not time-sensitive, I’ll aim to add the test cases over the
next few days.
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:2>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------

Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* owner: (none) => Senthil Kumar
* status: new => assigned

Comment:

Thanks for volunteering. There's some discussion in the linked PR review
with Clifford's initial investigation, if that helps any.
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:3>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Senthil Kumar):

Thanks Jacob. I will be creating a PR soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:4>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Senthil Kumar):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:5>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:6>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Senthil Kumar):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:7>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Senthil Kumar):

Based on your suggestion, I’ve revised the tests to ensure that the
correct caches are expired. As you mentioned above, I’ve also added a
failing test for db_returning_fields.

I have a question: now that we have a failing test, should I go ahead and
add db_returning_fields under FORWARD_PROPERTIES, or should I leave the
test as-is and mark it as expected to fail? I’d appreciate your guidance
on this.
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:8>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

I left a note that it looks like additional properties besides
`db_returning_fields` are worth clearing from the cache. Assuming we go
with a `subTest`-style test instead of writing separate cases for each, I
think fixing the behavior and testing in one commit would be simplest.
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:9>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:10>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: FORWARD_PROPERTIES, | Triage Stage: Accepted
Options |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Senthil Kumar):

* needs_better_patch: 1 => 0

Comment:

Thanks for the patience again. I have revised the unit tests based on the
suggested flow.
I have created a new PR as the previous PR's commit history got messed up.
I have also added the fix and all the tests are passing now.

Link to the new PR: https://github.com/django/django/pull/19645

Previous PR: https://github.com/django/django/pull/19484

I have also dropped a quick question about using apps.clear_cache() in the
PR. Hope to get some clarification on that. Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:11>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: FORWARD_PROPERTIES, | Triage Stage: Ready for
Options | checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:12>

[Django]

#36369: Missing test coverage for FORWARD_PROPERTIES entries
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Senthil
Type: | Kumar

Cleanup/optimization | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: FORWARD_PROPERTIES, | Triage Stage: Ready for
Options | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls <jacobtylerwalls@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"5ee651f2555f3258b136f6e8be90f018fd8ffbf0" 5ee651f]:
{{{#!CommitTicketReference repository=""
revision="5ee651f2555f3258b136f6e8be90f018fd8ffbf0"
Fixed #36369 -- Cleared additional cached properties in
apps.clear_cache().

Thanks Clifford Gama for the report.

Co-authored-by: Jacob Walls <jacobty...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36369#comment:13>