Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable better composition model #238

Open
taion opened this issue Feb 18, 2019 · 8 comments
Open

Enable better composition model #238

taion opened this issue Feb 18, 2019 · 8 comments

Comments

@taion
Copy link
Contributor

taion commented Feb 18, 2019

For larger APIs, the composition model in Flask-RESTy gets a bit messy. We often end up with multiple parallel complex inheritance trees in separate auth, schemas, and views modules, and a single view may compose in a half-dozen or more base classes and mixins. This is not ideal for understandability; the MRO gets complex enough that it's often not easy to figure out which method overrides which, or what super calls do.

For auth, many of these problems will be solved by #236. Outside of auth, though, we need to brainstorm further on how to make these work. One option is to move toward embedding class definitions for functionality. This would then allow patterns like:

class ChildView(ParentView):
    class Authorization(ParentView.Authorization):
        def authorize_delete_item(item):
            return False

    class Schema(ParentView.Schema):
        extra_field = fields.String()

Also, to limit the number of base classes in views, we could factor out classes to handle specific actions. For example, instead of something like:

class WidgetView(VersionedModelMixin, SoftDeleteMixin, GenericModelView):
    version_ignored_columns = ('version_id', 'version_committed_by')

    pass
class WidgetView(GenericModelView):
    class Updater(VersionedUpdater):
        ignored_columns = ('version_id', 'version_committed_by')

    class Deleter(SoftDeleteDeleter):
        pass

This could also enable patterns like:

class WidgetView(GenericModelView):
    class Model(SoftDeleteMixin.Model, db.Model):
        name = sa.Column(sa.String)

    Deleter = SoftDeleteMixin.Deleter

i.e. allow better co-location of mixin-type behavior across models and views.

This likely will require some version of #237.

@itajaja
Copy link
Member

itajaja commented Feb 19, 2019

not sure the colocation pattern actually gives you more expressiveness.

@taion
Copy link
Contributor Author

taion commented Feb 19, 2019

It's more like keeping things together. Separately handling "soft delete" or "entity create" mixins on models and views in different modules is annoying and seems potentially error-prone.

@sloria
Copy link
Member

sloria commented Feb 19, 2019

Seems like this could be done in a way that doesn't break backwards compat as well.

@sloria
Copy link
Member

sloria commented Aug 29, 2019

Doesn't solve the multi-inheritance issue, but you could get most of the colocation benefits by organizing modules by resource.

# widgets.py

class WidgetAuthorization(..., Auth):

class WidgetSchema(..., TableSchema):
    class Meta:
        table = Widget.__table__

class BaseWidgetView(... BaseView):

class WidgetListView(BaseWidgetView):

class WidgetView(BaseWidgetView):


api.add_resource('/api/widgets/', WidgetListView, WidgetView)

Update: Removed the model; models aren't 1:1 with resources, so probably still belong in a separate models.py

@itajaja
Copy link
Member

itajaja commented Aug 29, 2019

we should definitely try that to at least see how it feels. for big projects I think it might be better

@taion
Copy link
Contributor Author

taion commented Aug 29, 2019

Yusssssss

@sloria
Copy link
Member

sloria commented Aug 29, 2019

I'll write up a quick decision doc in notion and get yall to weigh in

@taion
Copy link
Contributor Author

taion commented Aug 29, 2019

The issue FWIW is that cross-module imports in Python suck because exports are values, not bindings. So there will be some awkwardness there.

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

No branches or pull requests

3 participants