Skip to content

Fix custom registry assignment in model_config and inheritance - #2090

Open
CAOShurong wants to merge 1 commit into
fastapi:mainfrom
CAOShurong:fix-custom-registry-model-config
Open

Fix custom registry assignment in model_config and inheritance#2090
CAOShurong wants to merge 1 commit into
fastapi:mainfrom
CAOShurong:fix-custom-registry-model-config

Conversation

@CAOShurong

Copy link
Copy Markdown

Summary

Fixes an issue where configuring a custom SQLAlchemy registry stored config_table into model_config["registry"] instead of config_registry, and ensures that concrete table models subclassing a custom base model are properly mapped.

Problem

  1. In SQLModelMetaclass.__new__:

    config_registry = get_config("registry")
    if config_registry is not Undefined:
        config_registry = cast(registry, config_registry)
        # If it was passed by kwargs, ensure it's also set in config
        new_cls.model_config["registry"] = config_table  # BUG: should be config_registry
        setattr(new_cls, "_sa_registry", config_registry)
        setattr(new_cls, "metadata", config_registry.metadata)
        setattr(new_cls, "__abstract__", True)

    new_cls.model_config["registry"] was assigned config_table (which is Undefined or a bool), rather than the actual config_registry object.

  2. Furthermore, because model_config is inherited by subclasses in Pydantic v2, reading get_config("registry") from inherited model_config would trigger the registry block on every subclass, unconditionally setting __abstract__ = True on concrete table classes inheriting from the custom base.

Solution

  1. Gate the custom registry definition to classes that explicitly declare registry via class kwargs (kwargs.get("registry")) or explicit class_dict["model_config"].
  2. Correctly assign new_cls.model_config["registry"] = config_registry.
  3. Add an end-to-end regression test in tests/test_main.py testing custom base registry definition, table subclassing, mapper initialization, and DB session operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants