Opened 6 years ago

Closed 2 years ago

Last modified 20 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 by Carlton Gibson, 6 years ago

Triage Stage: UnreviewedAccepted

Yes. Super.

comment:2 by Sergey Fedoseev, 6 years ago

Cc: Sergey Fedoseev added

comment:3 by Can Sarıgöl, 5 years ago

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

comment:4 by Carlton Gibson, 5 years ago

Needs documentation: set
Patch needs improvement: set

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

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

comment:6 by Himanshu Balasamanta, 3 years ago

@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 3 years ago by Himanshu Balasamanta (previous) (diff)

comment:7 by Sergey Fedoseev, 3 years ago

Cc: Sergey Fedoseev removed

comment:8 by Carlton Gibson, 3 years ago

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... 🤔 — 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. 🙂

Version 0, edited 3 years ago by Carlton Gibson (next)

in reply to:  8 comment:9 by Himanshu Balasamanta, 3 years ago

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 by Himanshu Balasamanta, 3 years ago

Cc: Himanshu Balasamanta added

comment:11 by Mariusz Felisiak, 2 years ago

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 by Mariusz Felisiak, 2 years ago

Has patch: set

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In e64919ae:

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

comment:14 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:15 by Mariusz Felisiak, 2 years ago

Resolution: fixed
Status: assignedclosed

comment:16 by GitHub <noreply@…>, 20 months ago

In 3afdc9e9:

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

Bug in cd1afd553f9c175ebccfc0f50e72b43b9604bd97.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 20 months ago

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