Opened 5 years ago

Closed 13 months ago

Last modified 6 months ago

#29799 closed New feature (fixed)

Allow registration and unregistration of lookups per field instances.

Reported by: Simon Charette Owned by: AllenJonathan
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: GSoC
Cc: Can Sarıgöl, Himanshu Balasamanta, Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Under certain circumstances it might be useful to register or unregister model field lookups on a per instance basis instead of a per class basis.

For example, it could be useful to disable costly lookups such as contains and icontains on some large table model CharFields without defining them using a CharField subclass to prevent them from being used in production. Similarly you might want to register or override a specific lookup for a third party app model field that doesn't allow swapping.

This was initially mentioned in #16187 which introduced this new lookup registration system.

https://code.djangoproject.com/ticket/16187#comment:14

Having the ability to hook a custom lookup to a given field instance would be really nice to have...

https://code.djangoproject.com/ticket/16187#comment:22

Add a way to register lookups to fields (I think either instances or classes is wanted behaviour).

But not discussed afterwhile. I believe class based registration was chosen because it was easier to implement but I agree that being able to register custom lookups per field instance would be good feature addition.

Change History (17)

comment:1 Changed 5 years ago by Carlton Gibson

Triage Stage: UnreviewedAccepted

Yes. Super.

comment:2 Changed 5 years ago by Sergey Fedoseev

Cc: Sergey Fedoseev added

comment:3 Changed 4 years ago by Can Sarıgöl

Cc: Can Sarıgöl added
Has patch: set

comment:4 Changed 4 years ago by Carlton Gibson

Needs documentation: set
Patch needs improvement: set

comment:5 Changed 4 years ago by Can Sarıgöl

Owner: changed from nobody to Can Sarıgöl
Status: newassigned

comment:6 Changed 18 months ago by Himanshu Balasamanta

@carltongibson , @charettes . I wanted to clarify exactly what all the scope of the ticket covers.
As per my understanding,

  • Enable registering and removal of a lookup for instances as it is with classes.
  • Enable lookups registered for the class to be unregistered (and maybe re-registered?) for the instances of that class.

Description of the functions would be as follows

  • get_lookups(selforclass)
    • when called by a field instance, returns all class and instance lookups currently registered for that field instance
    • when called by a class, returns all class lookups currently registered for that class
  • register_lookup(selforclass, lookup, lookup_name)
    • when called by a field instance, registers the lookup for that field instance
    • when called by a class, registers the lookup for that class
  • unregister_lookup(selforclass, lookup_name)
  1. when called by a field instance,
    • if the lookup is already registered with the instance, it gets removed,
    • if the lookup is registered with the instance class, that particular instance is exempted
    • else throws error
  2. when called by a class, if the lookup is already registered with the class, it is removed. Else error is thrown.

Doubts:

  1. Should class lookups that are unregistered for an instance be re-registered using register_lookup() itself?
  2. Should there be a method to specifically get lookups that were registered for the instance and not the class?
Last edited 18 months ago by Himanshu Balasamanta (previous) (diff)

comment:7 Changed 18 months ago by Sergey Fedoseev

Cc: Sergey Fedoseev removed

comment:8 Changed 18 months ago by Carlton Gibson

Hi Himanshu.

The rough outline looks right. There are various variations but, I'd like to add a custom lookup just for this model — the model being the bearer of field instances; I'd like to disable this lookup on this model, and so on.

The specific behaviours are hard to say definitively one way or the other until we get to the review stage. Looking at test cases, and seeing how the API looks at that point is much easier to speak clearly to.

(For example, I'm not sure what to say about trying to unregister a non-registered lookup on a class... — we could error, or we could pass: the end effect is that the lookup is not registered on the class... — My query would be why an error? 🤔 — it's the kind of detail we can settle with multiple sets of eyes at review time.)

I know you're looking at this targeting GSoC, so for the the sake of a proposal, noting the areas to be decided is OK. Showing that you've considered points already raised is important — make sure you looked at Simon's comment on your draft PR — I'm sure you did. Plus #33626 and the PR there you've made are all good signs — that's on my list to review next week.

I hope that helps. 🙂

Last edited 18 months ago by Carlton Gibson (previous) (diff)

comment:9 in reply to:  8 Changed 18 months ago by Himanshu Balasamanta

Replying to Carlton Gibson:

The rough outline looks right. There are various variations but, I'd like to add a custom lookup just for this model — the model being the bearer of field instances; I'd like to disable this lookup on this model, and so on.

Agreed.👍 It would be more user friendly.
So, along with register_lookup(self,lookup,lookup_name), I shall add a Model.register_lookup(field_name : str, lookup : Lookup, lookup_name : str) too.

I know you're looking at this targeting GSoC, so for the the sake of a proposal, noting the areas to be decided is OK. Showing that you've considered points already raised is important — make sure you looked at Simon's comment on your draft PR — I'm sure you did. Plus #33626 and the PR there you've made are all good signs — that's on my list to review next week.

I hope that helps. 🙂

Thank You!

comment:10 Changed 17 months ago by Himanshu Balasamanta

Cc: Himanshu Balasamanta added

comment:11 Changed 16 months ago by Mariusz Felisiak

Cc: Simon Charette added
Has patch: unset
Keywords: GSoC added
Needs documentation: unset
Owner: changed from Can Sarıgöl to AllenJonathan
Patch needs improvement: unset

comment:12 Changed 13 months ago by Mariusz Felisiak

Has patch: set

comment:13 Changed 13 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In e64919ae:

Refs #29799 -- Added more tests for registering lookups.

comment:14 Changed 13 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:15 Changed 13 months ago by Mariusz Felisiak

Resolution: fixed
Status: assignedclosed

comment:16 Changed 6 months ago by GitHub <noreply@…>

In 3afdc9e9:

Refs #29799 -- Added field instance lookups to suggestions in FieldErrors.

Bug in cd1afd553f9c175ebccfc0f50e72b43b9604bd97.

comment:17 Changed 6 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In be6a309:

[4.2.x] Refs #29799 -- Added field instance lookups to suggestions in FieldErrors.

Bug in cd1afd553f9c175ebccfc0f50e72b43b9604bd97.
Backport of 3afdc9e9b47d5bdd1bd653633b4cb2357478ade5 from main

Note: See TracTickets for help on using tickets.
Back to Top