Skip to content

Conversation

@BlayeeR
Copy link
Contributor

@BlayeeR BlayeeR commented May 3, 2024

This PR fixes a bug where in some scenarios, when using multiput on a models that have proxy submodel(a submodel without a db table) it would fail on trying to override superclass for that model instance(_multi_put_override_superclass method in the views.py file).

Example stacktrace of the error:

Traceback (most recent call last):
  File "/binder/tests/test_inheritance.py", line 93, in test_create_lion_with_animal_and_zoo
    response = self.client.put(
               ^^^^^^^^^^^^^^^^
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/test/client.py", line 1026, in put
    response = super().put(
               ^^^^^^^^^^^^
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/test/client.py", line 537, in put
    return self.generic(
           ^^^^^^^^^^^^^
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/test/client.py", line 609, in generic
    return self.request(**r)
           ^^^^^^^^^^^^^^^^^
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/test/client.py", line 891, in request
    self.check_exception(response)
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/test/client.py", line 738, in check_exception
    raise exc_value
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/binder/binder/views.py", line 553, in dispatch
    response = super().dispatch(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/binder/.eggs/Django-4.2.11-py3.12.egg/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/binder/binder/views.py", line 2686, in put
    return self.multi_put(request)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/binder/binder/views.py", line 2609, in multi_put
    objects, overrides = self._multi_put_override_superclass(objects)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/binder/binder/views.py", line 2329, in _multi_put_override_superclass
    subkey = subcls._meta.pk.remote_field.name
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'name'

Given the purpose of that method I think it is not needed to override the superclass with proxy subclass- it is essentialy the same model from the db perspective. Thats why when this happens I added continue statement in the subclass loop, so it tries to search for a valid subclass for overriding.

Infact the newly added test is not really needed. Adding Pet proxy model, that inherits from Animal model is enough for existing inheritance tests to break.

Additionaly after adding support for Django 4, the docker setup required bumping the version of Postgres container to 12 in order to work.

@BlayeeR BlayeeR added the bug label May 3, 2024
@BlayeeR BlayeeR requested review from daanvdk and stefanmajoor May 3, 2024 15:55
@BlayeeR BlayeeR self-assigned this May 3, 2024
@BlayeeR BlayeeR force-pushed the multiput-proxy-submodel-fix branch from fa02024 to 8db9639 Compare May 3, 2024 15:56
@BlayeeR BlayeeR force-pushed the multiput-proxy-submodel-fix branch from 8db9639 to eb46c18 Compare May 3, 2024 16:05
@BlayeeR
Copy link
Contributor Author

BlayeeR commented Jun 25, 2024

additional explanation of the issue:
the code in _multi_put_override_superclass as far as I understand replaces superclass with its subclass if possible. Lets say we have a Lion instance which is subclass to Animal superclass. Then it uses two tables in the db: lion table with pointer foreign key to animal table, and animal table. So when we save Animal directly, we check if its actually a polymorphism- a Lion class disguised as Animal class. If it is we replace it so we call save on Lion instead on Animal.

The thing is that with proxy model we have a superclass, but there is no pointer to it- because proxy model is pure python wrapper to existing model so that it can be extended with additional logic, have separate view etc. So if we try to save a model that has proxy submodel then it fails with 500 trying to get non existing remote_field

Local testing uses a different package install process then the CI. To
prevent us running postgres 12 default while local testing bu use 11
instead we must also use django 3.

To enable this we in the setup.py which is used for local testing
require a django 3 version instead of a django 3 or 4 version.
Since django 4 is only postgres 12+ compatible.

Co-authored-by: Bob Booij-Liewes <[email protected]>
@BBooijLiewes BBooijLiewes self-requested a review June 25, 2024 13:47
Copy link
Contributor

@BBooijLiewes BBooijLiewes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After changes all OK, lets rework setup.py testing later:
#243

@BBooijLiewes BBooijLiewes removed the request for review from stefanmajoor June 25, 2024 13:49
@BBooijLiewes BBooijLiewes merged commit 3085224 into master Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants