From b82a2d3a56234ee5c4c3baf06d873b28199f56ad Mon Sep 17 00:00:00 2001 From: Carter Costic Date: Wed, 29 Jan 2025 22:33:19 -0500 Subject: [PATCH 1/3] fix: propose change for inheriting override in the presence of include_fk=False --- src/marshmallow_sqlalchemy/schema.py | 32 +++++++++++++++++++++++++--- tests/test_sqlalchemy_schema.py | 29 +++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/src/marshmallow_sqlalchemy/schema.py b/src/marshmallow_sqlalchemy/schema.py index ba7b934..46cf3f8 100644 --- a/src/marshmallow_sqlalchemy/schema.py +++ b/src/marshmallow_sqlalchemy/schema.py @@ -1,10 +1,11 @@ from __future__ import annotations +import inspect from typing import TYPE_CHECKING, Any, cast import sqlalchemy as sa from marshmallow.fields import Field -from marshmallow.schema import Schema, SchemaMeta, SchemaOpts +from marshmallow.schema import Schema, SchemaMeta, SchemaOpts, _get_fields from .convert import ModelConverter from .exceptions import IncorrectSchemaTypeError @@ -118,7 +119,7 @@ def get_declared_fields( cls_fields, # Filter out fields generated from foreign key columns # if include_fk is set to False in the options - mcs._maybe_filter_foreign_keys(inherited_fields, opts=opts), + mcs._maybe_filter_foreign_keys(inherited_fields, opts=opts, klass=klass), dict_cls, ) fields.update(mcs.get_declared_sqla_fields(fields, converter, opts, dict_cls)) @@ -159,6 +160,7 @@ def _maybe_filter_foreign_keys( fields: list[tuple[str, Field]], *, opts: SQLAlchemySchemaOpts, + klass: SchemaMeta, ) -> list[tuple[str, Field]]: if opts.model is not None or opts.table is not None: if not hasattr(opts, "include_fk") or opts.include_fk is True: @@ -168,7 +170,31 @@ def _maybe_filter_foreign_keys( for column in sa.inspect(opts.model or opts.table).columns # type: ignore[union-attr] if column.foreign_keys } - return [(name, field) for name, field in fields if name not in foreign_keys] + + schema_overrides = [ + base + for base in inspect.getmro(klass) + if issubclass(base, Schema) + and not issubclass(base, SQLAlchemyAutoSchema) + ] + + def is_overridden(field: str) -> bool: + return any( + field + in [ + name + for name, _ in _get_fields( + getattr(base, "_declared_fields", base.__dict__) + ) + ] + for base in schema_overrides + ) + + return [ + (name, field) + for name, field in fields + if name not in foreign_keys or is_overridden(name) + ] return fields diff --git a/tests/test_sqlalchemy_schema.py b/tests/test_sqlalchemy_schema.py index b5a1623..93e1a61 100644 --- a/tests/test_sqlalchemy_schema.py +++ b/tests/test_sqlalchemy_schema.py @@ -681,6 +681,34 @@ class Meta(TeacherSchema.Meta): assert "current_school_id" in schema2.fields +def test_auto_schema_with_model_allows_schema_extensions_to_override_include_fk_with_explicit_inherited_field( + models, +): + class OverrideSchema(Schema): + current_school_id = fields.Integer() + + class TeacherSchema(SQLAlchemyAutoSchema, OverrideSchema): + class Meta: + model = models.Teacher + + schema = TeacherSchema() + assert "current_school_id" in schema.fields + + +def test_auto_schema_with_table_allows_schema_extensions_to_override_include_fk_with_explicit_inherited_field( + models, +): + class OverrideSchema(Schema): + current_school_id = fields.Integer() + + class TeacherSchema(SQLAlchemyAutoSchema, OverrideSchema): + class Meta: + table = models.Teacher.__table__ + + schema = TeacherSchema() + assert "current_school_id" in schema.fields + + def test_auto_field_does_not_accept_arbitrary_kwargs(models): if int(version("marshmallow")[0]) < 4: from marshmallow.warnings import RemovedInMarshmallow4Warning @@ -695,6 +723,7 @@ class Meta: model = models.Course name = auto_field(description="A course name") + else: with pytest.raises(TypeError, match="unexpected keyword argument"): From 95eda67e1b236c71f8ce0850c2210da3d258d832 Mon Sep 17 00:00:00 2001 From: Steven Loria Date: Mon, 10 Feb 2025 17:28:11 -0500 Subject: [PATCH 2/3] Improve naming --- src/marshmallow_sqlalchemy/schema.py | 12 +++++++----- tests/test_sqlalchemy_schema.py | 14 ++++++++------ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/marshmallow_sqlalchemy/schema.py b/src/marshmallow_sqlalchemy/schema.py index 46cf3f8..30452c0 100644 --- a/src/marshmallow_sqlalchemy/schema.py +++ b/src/marshmallow_sqlalchemy/schema.py @@ -171,14 +171,14 @@ def _maybe_filter_foreign_keys( if column.foreign_keys } - schema_overrides = [ + non_auto_schema_bases = [ base for base in inspect.getmro(klass) if issubclass(base, Schema) and not issubclass(base, SQLAlchemyAutoSchema) ] - def is_overridden(field: str) -> bool: + def is_declared_field(field: str) -> bool: return any( field in [ @@ -187,20 +187,22 @@ def is_overridden(field: str) -> bool: getattr(base, "_declared_fields", base.__dict__) ) ] - for base in schema_overrides + for base in non_auto_schema_bases ) return [ (name, field) for name, field in fields - if name not in foreign_keys or is_overridden(name) + if name not in foreign_keys or is_declared_field(name) ] return fields class SQLAlchemyAutoSchemaMeta(SQLAlchemySchemaMeta): @classmethod - def get_declared_sqla_fields(cls, base_fields, converter, opts, dict_cls): + def get_declared_sqla_fields( + cls, base_fields, converter: ModelConverter, opts, dict_cls + ): fields = dict_cls() if opts.table is not None: fields.update( diff --git a/tests/test_sqlalchemy_schema.py b/tests/test_sqlalchemy_schema.py index 93e1a61..a3f5314 100644 --- a/tests/test_sqlalchemy_schema.py +++ b/tests/test_sqlalchemy_schema.py @@ -681,29 +681,31 @@ class Meta(TeacherSchema.Meta): assert "current_school_id" in schema2.fields -def test_auto_schema_with_model_allows_schema_extensions_to_override_include_fk_with_explicit_inherited_field( +def test_auto_schema_with_model_can_inherit_declared_field_for_foreign_key_column_when_include_fk_is_false( models, ): - class OverrideSchema(Schema): + class BaseTeacherSchema(Schema): current_school_id = fields.Integer() - class TeacherSchema(SQLAlchemyAutoSchema, OverrideSchema): + class TeacherSchema(BaseTeacherSchema, SQLAlchemyAutoSchema): class Meta: model = models.Teacher + include_fk = False schema = TeacherSchema() assert "current_school_id" in schema.fields -def test_auto_schema_with_table_allows_schema_extensions_to_override_include_fk_with_explicit_inherited_field( +def test_auto_schema_with_table_can_inherit_declared_field_for_foreign_key_column_when_include_fk_is_false( models, ): - class OverrideSchema(Schema): + class BaseTeacherSchema(Schema): current_school_id = fields.Integer() - class TeacherSchema(SQLAlchemyAutoSchema, OverrideSchema): + class TeacherSchema(BaseTeacherSchema, SQLAlchemyAutoSchema): class Meta: table = models.Teacher.__table__ + include_fk = False schema = TeacherSchema() assert "current_school_id" in schema.fields From 004214e673608c0f649e5f12f68dc0cc65719857 Mon Sep 17 00:00:00 2001 From: Steven Loria Date: Mon, 10 Feb 2025 17:29:57 -0500 Subject: [PATCH 3/3] Update changelog --- CHANGELOG.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f32af65..60014f8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,6 +1,15 @@ Changelog --------- +(unreleased) +++++++++++++ + +Bug fixes: + +* Fix inheritance of declared fields that match then name of a foreign key column + when the ``include_fk`` option is set to ``False`` (:pr:`657`). + Thanks :user:`carterjc` for the PR. + 1.4.0 (2025-01-19) ++++++++++++++++++