Opened 8 years ago

Closed 4 years ago

Last modified 4 years ago

#5968 closed New feature (fixed)

Registering/Unregistering multiple models fails

Reported by: anderso Owned by: aaugustin
Component: contrib.databrowse Version: master
Severity: Normal Keywords:
Cc: 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

Registering and unregistering multiple models fails. Calling for example

databrowse.site.register([model1, model2])

triggers this error: issubclass() arg 1 must be a class.

The patch fixes this by checking if the argument is a type before calling issubclass. The call to issubclass is now not strictly necessary (unless someone would want to pass in an iterable of models that is also type) but I kept it for clarity.

Attachments (6)

allowiterable.diff (1.3 KB) - added by anderso 8 years ago.
allowiterable_2.diff (1006 bytes) - added by anderso 8 years ago.
5968-1.diff (1.2 KB) - added by mattmcc 7 years ago.
Patch using the same test as AdminSite.register
5968-2.diff (2.2 KB) - added by mattmcc 7 years ago.
Add doc update
5968-3.diff (5.6 KB) - added by jamesp 4 years ago.
Adding tests
5968-4.patch (6.0 KB) - added by aaugustin 4 years ago.

Download all attachments as: .zip

Change History (25)

Changed 8 years ago by anderso

comment:1 Changed 8 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set

Checking for something being an instance of type is usually a sign of things going wrong. What are you actually trying to achieve here? Allowing an iterable as an argument? If so, then check to see if it's an iterable, not if it's a type, which is way too broad.

However, even that is probably not the best approach. If you're going to try and allow multiple models to be registered, just let register() take a *args parameter so the user doesn't have to needlessly wrap things in a tuple or list and can just write

databrowse.site.register(model1, model2)

comment:2 Changed 8 years ago by anderso

The intent of the original author was to allow either a Model or an iterable of Models, but the check for identifying the actual case was faulty. An *args parameter would probably be nicest but the function has this signature, which doesn't make that a pretty option.

def register(self, model_or_iterable, databrowse_class=None, **options):

I have changed the patch to check if it's not an iterable instead of checking if it's a Model. By the way, is hasattr(o, "iter") the best way to check for an iterable?

Changed 8 years ago by anderso

comment:3 Changed 8 years ago by mtredinnick

  • Needs documentation set
  • Triage Stage changed from Unreviewed to Accepted

Since you're doing a for-loop over the iterator, it must have an __iter__ or __getitem__ method, so the test you're doing is fine.

Leave the patch as it is for now, but I'm still tempted to move to the *args format. It would be mostly backwards compatible, the only difference being that databrowse_class would have to be specified as a keyword argument, but we can enforce that and that parameter isn't documented, so arguably isn't part of the public API (yet, at least).

I'm still a little concerned about how you're going about the test here. Currently we check that the input must be a model. This patch changes that so that site.register(int) won't raise the error it does now and will only cause problems later. I think it's important to keep the test for models in there somewhere. Presumably the end result is to allow multiple models to be registered at once. The iterable is just a way to achieve that, not a requirement, right? After all, given an iterable, you can still make it work with the *args format by calling register(*list(my_iter)). Thus my slight preference for the *args version when we can just iterate over args inside the register() method.

Anyway, let's leave it as is for now and we can tweak it when it comes time to commit. I think the idea is reasonable, however we end up imlpementing it.

comment:4 Changed 8 years ago by anderso

Yes I believe it's just a convenience for adding several models at once. The use case where you already have a list of Models is probably rare (and if you did you could just use the * syntax in the call). Verifying that the arguments really are Models might be nice since this is the main public api of databrowse.

If we were to rethink the api, maybe it would make sense to provide registering at the application level in addition to individual models. I imagine this is how it looks for many users:

from myapp.models import ModelA, ModelB, ModelC, ModelD, ModelE
databrowse.site.register(ModelA)
databrowse.site.register(ModelB)
databrowse.site.register(ModelC)
databrowse.site.register(ModelD)
databrowse.site.register(ModelE)

or with the undocumented iterable approach:

from myapp.models import ModelA, ModelB, ModelC, ModelD, ModelE
databrowse.site.register([ModelA, ModelB, ModelC, ModelD, ModelE])

maybe something like this would be a nice alternative:

import myapp
databrowse.site.register_application(myapp)

Makes sense considering a main use case of databrowse is to provide a quick overview of the data for developers.

comment:5 Changed 7 years ago by mattmcc

Is there any reason not to fix this bug by using the same test that AdminSite uses?

isinstance(model_or_iterable, ModelBase)

Changed 7 years ago by mattmcc

Patch using the same test as AdminSite.register

Changed 7 years ago by mattmcc

Add doc update

comment:6 Changed 7 years ago by mattmcc

  • milestone set to 1.0
  • Needs documentation unset
  • Patch needs improvement unset

comment:7 Changed 7 years ago by mtredinnick

  • milestone changed from 1.0 to post-1.0

This is a feature addition, not a bug fix.

comment:8 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:9 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to New feature

comment:10 Changed 4 years ago by julien

  • Easy pickings unset
  • Needs tests set

Changed 4 years ago by jamesp

Adding tests

comment:11 Changed 4 years ago by jamesp

  • UI/UX unset

Updated to trunk@[16797] and added tests (I hope checking to ensure models were correctly added to databrowse.site.registry is adequate).

comment:12 Changed 4 years ago by ptone

Databrowse is now deprecated, see #16907

however this has a recent, solid patch, and should still be considered for inclusion while databrowse remains

comment:13 Changed 4 years ago by PaulM

  • Needs tests unset
  • Patch needs improvement set

I don't see anything wrong with this patch, and will be happy to add it as a minor improvement even though databrowse is deprecated. The tests are ok (since databrowse is otherwise untested). Unfortunately, the patch needs to be updated again to catch the pending deprecation warning. Feel free to mark it as RFC again once that's taken care of.

comment:14 Changed 4 years ago by aaugustin

  • Patch needs improvement unset

While the current code apparently intends to make it possible to register or unregister multiple models at once by passing a list, this ticket shows that it doesn't work, and it was never documented.

Like Malcolm, I'd prefer to move to the *args format, as demonstrated in the patch I'm attaching. Also, contrib apps bundle their own tests.

Changed 4 years ago by aaugustin

comment:15 Changed 4 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:16 Changed 4 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

It seems in good shape! Thanks.

comment:17 Changed 4 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

In [17405]:

Fixed #5968 -- Allowed (un-)registering with databrowse several models at once.

comment:18 Changed 4 years ago by aaugustin

In [17406]:

Added basic tests for databrowse. Refs #5968.

comment:19 Changed 4 years ago by aaugustin

The changes to tests/runtests.py should have gone in the second commit, sorry.

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