Model.from_db() overrides without fetch_mode argument crash with TypeError on any query

[Adam Johnson]

The fetch modes feature added in Django 6.1 (#28586) made every query
iteration path pass a new fetch_mode keyword argument to
Model.from_db(). Any model that overrides from_db() with the
signature documented in the model instance reference —
from_db(cls, db, field_names, values), including the documented
_loaded_values example — now crashes on any database fetch:

TypeError: MyModel.from_db() got an unexpected keyword argument 'fetch_mode'

This worked on Django 6.0 and all earlier versions.

The proposed fix is to deprecate from_db() overrides that do not accept the new fetch_mode keyword argument, using django.utils.inspect to detect the override signature and conditionally pass fetch_mode or not, emitting a RemovedInDjango70Warning for old-signature overrides.

[Jacob Walls]

Thanks for the ticket. Clearly we need to update the documented example with a versionchanged:: 6.1 box, but looking at the provided patch, doesn't this just shift the cheese around in a way that will cause problems for ModelIterable.__iter__() overrides?

I'm struggling to see the benefit of a shim here, and for me the bar is even higher now that we're post-release. I would also want to see a microbenchmark to see what we're paying for the inspect calls. I'll close for now, but I can take another look if you would like to add some info.

[Jacob Walls]

I just noticed the documentation example has the following caveat. It just gives super() instead of a variant of super().from_db(*args, **kwargs), but even so it feels like enough of a disclaimer:

    @classmethod
    def from_db(cls, db, field_names, values, *, fetch_mode=None):
        # Default implementation of from_db() (subject to change and could
        # be replaced with super()).

[Adam Johnson]

Replying to Jacob Walls:

...but looking at the provided patch, doesn't this just shift the cheese around in a way that will cause problems for ModelIterable.__iter__() overrides?

Not really: ModelIterable is not a public API.

I'm struggling to see the benefit of a shim here

I think we should do it to conform to the deprecation policy: all public API changes go through it. I appreciate that the method is rarely overridden, and if so, failures will be spotted quickly. But I think making a decision to bypass the policy for any particular documented method would need the steering council’s input.

and for me the bar is even higher now that we're post-release.

The call-time check is forwards-compatible, so this change can only make things easier for folks yet to upgrade.

I would also want to see a microbenchmark to see what we're paying for the inspect calls.

Signatures are cached in django.utils.inspect so the overhead is small:

In [1]: from django.utils.inspect import func_accepts_kwargs, func_supports_parameter

In [2]: def from_db(cls, db, field_names, values): pass  # old style

In [3]: %timeit func_supports_parameter(from_db, "fetch_mode") or func_accepts_kwargs(from_db)
977 ns ± 12.2 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: def from_db(cls, db, field_names, values, *, fetch_mode=None): pass  # new style

In [5]: %timeit func_supports_parameter(from_db, "fetch_mode") or func_accepts_kwargs(from_db)
482 ns ± 1.41 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

I just noticed the documentation example has the following caveat.

My reading of that is an attempt to educate on the default behaviour while also showing how to override, and doesn’t in any way advertise that the method will change signature without deprecation.

[Jacob Walls]

Replying to Adam Johnson:

I think we should do it to conform to the deprecation policy: all public API changes go through it.

For sure, I wasn't suggesting to ignore the policy: I just had not encountered a case where we failed to document **kwargs in an example of overriding a method, and I wasn't sure how the policy would apply in such a case. As your report points out, the fact that Django forwards the new argument on in every iteration path is important. So let's treat this as a breaking change. I'll reopen your PR.

[Jacob Walls <jacobtylerwalls@…>]

In d992705f:

Fixed #37259 -- Restored support for old-signature Model.from_db() overrides.

The fetch modes feature made all query iteration paths (ModelIterable,
RawModelIterable, and RelatedPopulator) call Model.from_db() with a
fetch_mode keyword argument. This crashed with a TypeError for models
overriding from_db() with the previously documented signature
from_db(cls, db, field_names, values).

Query iteration now detects, once per query rather than per row, whether
the model's from_db() accepts the fetch_mode keyword argument (or
kwargs) and calls old-style overrides without it, emitting a
RemovedInDjango70Warning asking authors to add the argument.

Regression in e097e8a12f21a4e92594830f1ad1942b31916d0f.

[Jacob Walls <jacobtylerwalls@…>]

In cabad83:

[6.1.x] Fixed #37259 -- Restored support for old-signature Model.from_db() overrides.

The fetch modes feature made all query iteration paths (ModelIterable,
RawModelIterable, and RelatedPopulator) call Model.from_db() with a
fetch_mode keyword argument. This crashed with a TypeError for models
overriding from_db() with the previously documented signature
from_db(cls, db, field_names, values).

Query iteration now detects, once per query rather than per row, whether
the model's from_db() accepts the fetch_mode keyword argument (or
kwargs) and calls old-style overrides without it, emitting a
RemovedInDjango70Warning asking authors to add the argument.

Regression in e097e8a12f21a4e92594830f1ad1942b31916d0f.

Backport of d992705f9eb56199dc474b77af16474e5ce3d2ab from main.