API to customize the list of TYPE_MAPPING

[stephane]

I use sqlalchemy_utils which provide ChoiceType field (and many others).

I'm able to create a new Marshmallow field (Field) to handle this new field with proper _serialize/_deserialize functions but instead to override the attributes one by one in each schema I defined I want to update the list type_mapping in ModelConverter.

I didn't find how to access it properly, ModelConverter accepts schema_cls attribute at init https://github.com/marshmallow-code/marshmallow-sqlalchemy/blob/dev/marshmallow_sqlalchemy/convert.py#L76 but I'm not able to figure out how to use it properly with all these meta classes ;)

So for now I rely on a monkey patch:

ModelConverter.SQLA_TYPE_MAPPING.update({
    ChoiceType: ChoiceTypeSchema
})

PS: I use flask-marshmallow in my app.

[georgexsh]

@stephane why not subclass ModelConverter and extent type_mapping?

class MyModelConverter(marshmallow_sqlalchemy.ModelConverter):

    @property
    def type_mapping(self):
        mapping = super(MyModelConverter, self).type_mapping
        mapping.update({
            datetime.datetime: UnixTiemstampField,
        })
        return mapping


class SomeSchema(ma.ModelSchema):
    class Meta:
       model_converter = MyModelConverter

but as I map ChoiceType to EnumField from https://github.com/justanr/marshmallow_enum, the conversion need access model property object, I need overridden property2field:

class MyModelConverter(marshmallow_sqlalchemy.ModelConverter):

    def property2field(self, prop, instance=True, **kwargs):
        if not hasattr(prop, 'direction'):
            column = prop.columns[0]
            if isinstance(column.type, sqlalchemy_utils.ChoiceType):
                assert instance
                field_kwargs = self._get_field_kwargs_for_property(prop)
                field_kwargs.update(kwargs)
                return EnumField(column.type.choices, **field_kwargs)
        return super(MyModelConverter, self).property2field(prop, instance, **kwargs)

+1 for a more elegant API for mapping customize.

[sloria]

Yeah, I think it might make sense to have a class Meta option to override type mappings. Perhaps we could call it type_mapping_overrides.

I would certainly review and merge a PR implementing this.

[frol]

#83 added support for TypeDecorated fields, which are heavily used in SQLAlchemy-Utils, so your code shouldn't fail on start now (if you install Marshmallow-sqlalchemy >=0.12.0-dev version)), so now you can just override the auto-generated field defining it explicitly on your Schema class.

[ThiefMaster]

I don't think it makes sense to specify a custom converter via Meta if you have a custom SQLAlchemy type that's used everywhere in your application. There should be a clean/documented way to globally register a new type.

[cancan101]

Following up on this, it would be great if there were an easy to have sa.Enum map to an EnumField. Perhaps that logic could be default in marshmallow-sqlalchemy if the marshmallow_enum package is installed.

[sloria]

I think adding adding a type_mapping_overrides class Meta option that gets merged with ModelConverter.SQLA_TYPE_MAPPING is a good first step that gets us 90% of the way.

For the EnumField use case, perhaps we could support passing functions:

class Meta:
    type_mapping_overrides = {
        sqlalchemy_utils.ChoiceType: lambda column, kwargs: EnumField(column.type.choices, **kwargs)
    }

[sloria]

I'm considering making the converter class a class member of the schema rather than class Meta option, i.e.

class SQLAlchemySchema(...):
    Converter = ModelConverter

This could simplify overriding converter methods and attributes, incl. the type mapping.

class MyBaseSchema(SQLAlchemySchema):
    class Converter(SQLAlchemySchema.Converter):
        SQLA_TYPE_MAPPING = {
            **SQLAlchemySchema.Converter.SQLA_TYPE_MAPPING,
            datetime.datetime: UnixTimeStampField,
        }

Thoughts?

[taion]

I'm not sure how I feel about moving the converter off of "meta". It seems to me that it intuitively should be "next to" the declaration of the model or table to which the schema is bound.

If those live on the Meta, then shouldn't the converter declaration also live there?

[sloria]

The converter isn't something that will typically differ per subclass. I'd expect that it would be customized once at the base Schema level. In that sense, it's conceptually closer to OPTIONS_CLASS than it is to table or model.

[taion]

Is the only thing this saves in practice not having to use a custom schema opts subclass? I guess I don't see offhand why it should be different from, say, unknown, which we in fact do only set once on the base schema...

I wonder, though, if it would be better for SQLA_TYPE_MAPPING to live on the schema, given that TYPE_MAPPING already does.

[sloria]

Is the only thing this saves in practice not having to use a custom schema opts subclass?

Not exactly. You can still get away without a custom options class, as in the comment above.

I guess I don't see offhand why it should be different from, say, unknown, which we in fact do only set once on the base schema...

Fair point. More precisely, I think the distinction is that class Meta options are related to serde behavior and OPTIONS_CLASS and Converter are related to how schemas are constructed.

The goal is to make two common use cases more ergonomic:

1. Overriding the SQLA->Field mapping
2. Overriding converter behavior

Adding Meta.type_mapping_overrides or putting SQLA_TYPE_MAPPING on the schema does make 1. easier but doesn't address 2. Given how tightly related those two use cases are, it seems like they should stay on the same class?

[taion]

Well, would it be reasonable to do both? Can the converter look at the schema to get the type mapping? It feels more weird for type mapping to work differently in the SQLAlchemy context than for configuring the converter to take a few extra steps, IMO.

[zezic]

So... as long as the API is not implemented yet what's the simplest way to extend the SQLA_TYPE_MAPPING for marshmallow-sqlalchemy v0.23.1?

[space-nuko]

Same issue, can this be done