pallets-eco / flask-admin

Simple and extensible administrative interface framework for Flask
https://flask-admin.readthedocs.io
BSD 3-Clause "New" or "Revised" License
5.78k stars 1.57k forks source link

`SynonymProperty` and `hybrid_property` aren't displayed in the model form #78

Closed dmedvinsky closed 11 years ago

dmedvinsky commented 12 years ago

I used to use db.synonym from sqlalchemy in my model with Flask-Admin 1.0.1. Everything worked fine.

Recently I tried it with latest master and synonyms aren't rendered in the model form anymore as default converter kind of ignores them now.

I also tried using hybrid_property as it is superset of synonym. They aren't rendered in the model form at all as they aren't listed in the mapper.iterate_properties.

Any ideas? Because I'm out of them right now after debugging this (new for me stuff) for a couple of hours.

mrjoes commented 12 years ago

Can you provide small example which illustrates the problem?

dmedvinsky commented 12 years ago

I can try. :-)

models.py:

from sqlalchemy.ext import hybrid
from myapp import db

class Post(db.Model):
    id = db.Column(db.Integer, primary_key=True)
    content = db.Column(db.UnicodeText())
    _tags = db.Column('tags', db.Unicode(512))

    @hybrid.hybrid_property
    def tags(self):
        return self._tags

    @tags.setter  # NOQA
    def tags(self, tags):
        self._tags = tags
        # Creating tags in database, blog-tags m2m relation, and maybe something else.

admin.py:

from flask.ext import wtf
from flask.ext.admin.contrib.sqlamodel import ModelView
from flask.ext.babel import lazy_gettext

from . import models

class PostAdmin(ModelView):
    list_columns = ['content', 'tags']
    form_columns = ['content', 'tags']
    form_overrides = dict(tags=wtf.TextField)

    def __init__(self, *args, **kwargs):
        super(PostAdmin, self).__init__(models.Post, *args, **kwargs)

If I run it like this, I get a KeyError in the following line:

properties = ((x, temp[x]) for x in only)

Because this property isn't returned by the iterate_properties iterator.

See https://github.com/mrjoes/flask-admin/blob/56fc52b55dc602820baaa3d7a19cd8d46cf858e3/flask_admin/contrib/sqlamodel/form.py#L280

What I used before was pretty much the same only instead of hybrid_property stuff I used

tags = db.synonym('_tags', descriptor=property(_get_tags, _set_tags))

And this way, with Flask-Admin 1.0.1, it rendered the TextField all right.

techniq commented 12 years ago

One thing that should be added is a check that temp has the key (temp is the tuple to dict temporary variable of mapper.iterate_properties)

I looked at changing

properties = ((x, temp[x]) for x in only)

to

properties = ((x, temp[x]) for x in only if hasattr(temp, x))

but the following test fails, which I don't understand.

1) FAIL: flask_admin.tests.test_sqlamodel.test_non_int_pk

   Traceback (most recent call last):
    /Users/smlynch/Development/virtualenv/flask-admin/lib/python2.7/site-packages/nose/case.py line 197 in runTest
      self.test(*self.arg)
    test_sqlamodel.py line 322 in test_non_int_pk
      eq_(rv.status_code, 302)
   AssertionError: 200 != 302

This won't fix the issue, but won't break where it shouldn't (the property just won't be shown).

dmedvinsky commented 12 years ago

@techniq I thought about it, too, but then I figured, why do this? If your model form lists the field that doesn't exist in the model, it is kind of exceptional situation. Why leave it silent? Errors should never pass silently, after all.

techniq commented 12 years ago

@dmedvinsky I agree. Might be worth wrapping in a try/except for a better error / description, but failing is the right outcome.

dmedvinsky commented 12 years ago

Funny thing, if I put '_tags' in form_columns the field isn't displayed either, only this time with no error. Gonna pdb some more.

dmedvinsky commented 12 years ago

Well, apparently that is because of the name that starts with an underscore. Though I'm not sure yet why does this happen because '_tags' go into field_dict when the form class is created all right.

dmedvinsky commented 12 years ago

Well, I managed to solve my particular issue with such patch:

diff --git a/flask_admin/contrib/sqlamodel/form.py b/flask_admin/contrib/sqlamodel/form.py
index 156f4de..5666627 100644
--- a/flask_admin/contrib/sqlamodel/form.py
+++ b/flask_admin/contrib/sqlamodel/form.py
@@ -276,8 +276,15 @@ def get_form(model, converter,
     properties = ((p.key, p) for p in mapper.iterate_properties)
     if only:
         # Filter properties while maintaining property order in 'only' list
+        def find_property(xs, x):
+            if x in temp:
+                return temp[x]
+            for y in xs.itervalues():
+                if hasattr(y, 'columns') and y.columns[0].key == x:
+                    return y
+            raise ValueError('Field %s does not exist in mapper' % x)
         temp = dict(properties)
-        properties = ((x, temp[x]) for x in only)
+        properties = ((x, find_property(temp, x)) for x in only)
     elif exclude:
         properties = (x for x in properties if x[0] not in exclude)

But this is an ad-hocish hack which won't work in general case. It would only work if mapped column has the same name as hybrid property. That's why I'm not filing this as a pull request. I'm desperate for the ideas.

mrjoes commented 12 years ago

I'll take a look over weekend. I doubt it has something to do with new form generation logic, as wtforms.ext.sqlalchem also uses iterate_properties.

So, it is quite possible that you installed newer version of SQLAlchemy and it stopped returning synonym properties from iterate_properties helper.

dmedvinsky commented 12 years ago

I highly doubt that because pip freeze gives me SQLAlchemy==0.7.8, which was published in PyPI on 2012-06-17, and the first commit in my repo is dated 2012-08-10. Also, all the sqlalchemy files in my virtualenv are dated 2012-08-10, too. So I assume it never changed.

mrjoes commented 12 years ago

Can you try your model with standard wtforms sqlalchemy converter?

https://bitbucket.org/simplecodes/wtforms/src/113994790508/wtforms/ext/sqlalchemy/orm.py#cl-253

dmedvinsky commented 12 years ago

It raises no errors, but sadly it doesn't yield me a tags field either.

Here's what I did:

>>> from wtforms.ext.sqlalchemy import orm
>>> from blog.models import Post
>>> from myapp import db
>>> f = orm.model_form(Post, db_session=db.session, only=['content', 'tags'])
>>> f.tags
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AttributeError: type object 'PostForm' has no attribute 'tags'
dmedvinsky commented 12 years ago

Awesome, thanks. Looks fine to me.

jvholst commented 12 years ago

Not working for me, hasattr and p.property fail (p class = sqlalchemy.orm.attributes.hybrid_propertyProxy).

dmedvinsky commented 12 years ago

@jvholst Can you provide some code samples you're using?

jvholst commented 11 years ago

@dmedvinsky I've posted an example of the issue at https://gist.github.com/3969681. Adding value to form_columns in AnimalFactInlineForm triggers the error "ValueError: Invalid model property name <class 'main.AnimalFact'>.value". I'm using Flask-Admin 1.0.3 (via git). Any help much appreciated!

mrjoes commented 11 years ago

OK, I know why it happens.

For @dmedvinsky, property getter was returning "proper" SQLAlchemy property every time. So, Flask-Admin has enough information to figure out how to work with the property.

In your case, default returned value is None and Flask-Admin can't figure out what to do with this property. Your approach will work fine in list view, but not for form scaffolding. I'm not sure how to support it - there's no way Flask-Admin can guess which wtform field to use based on return value of your getter.

Easiest way to have it working - contribute 'value' wtform field after form was constructed. Unfortunately, contributing inline models is not as flexible as I'd want it to be, so I will work on that.

mrjoes commented 11 years ago

@jvholst - I pushed some changes and you can now customize your inline form like this: https://gist.github.com/3993803

jvholst commented 11 years ago

Excellent, it works - thanks a lot!

lu-zero commented 11 years ago

the flask-admin 1.0.4 seems to fail to render those hybrid_properties.

class Invoice(db.Model): id = db.Column(db.Integer, primary_key=True) creation = db.Column(db.Date, default=datetime.now) due = db.Column(db.Date) rationale = db.Column(db.Text(256)) currency = db.Column(db.Enum("Euro", "USD", "GBP", name="Currency")) customer_id = db.Column(db.Integer, db.ForeignKey('customer.id')) customer = db.relationship('Customer', backref=db.backref('invoices', lazy='dynamic')) project_id = db.Column(db.Integer, db.ForeignKey('project.id')) project = db.relationship('Project', backref=db.backref('invoices', lazy='dynamic'))

@hybrid_property
def amount(self):
    return sum(item.amount for item in self.items)

@hybrid_property
def vat(self):
    return sum(item.vat for item in self.items)

@amount.expression
def amount(cls):
    return select([func.sum(InvoiceItem.amount)]).\
            where(InvoiceItem.invoice_id==cls.id).\
            label('amount')

@vat.expression
def vat(cls):
    return select([func.sum(InvoiceItem.vat)]).\
            where(InvoiceItem.invoice_id==cls.id).\
            label('vat')
mrjoes commented 11 years ago

Flask-Admin can only display model properties which map to the columns.

In your case, it is select statement and Flask-Admin doesn't know what to do with it.

I will reopen the issue, as Flask-Admin should be able to display them in the list view.

mrjoes commented 11 years ago

OK, it works as expected, but you need to explicitly specify it:

class User(db.Model):
    id = db.Column(db.Integer, primary_key=True)
    number = db.Column(db.Integer)

    @hybrid_property
    def test(self):
        return self.number + 1

    # Required for administrative interface
    def __unicode__(self):
        return self.number

class UserAdmin(sqlamodel.ModelView):
    list_columns = ('number', 'test')

You can also make it sortable:

class UserAdmin(sqlamodel.ModelView):
    list_columns = ('number', 'test')
    sortable_columns = (('test', 'number'),)
lu-zero commented 11 years ago

Thank you! I'll update soon.

On Tue, Mar 19, 2013 at 8:31 PM, Serge S. Koval notifications@github.comwrote:

OK, it works as expected, but you need to explicitly specify it:

class User(db.Model): id = db.Column(db.Integer, primary_key=True) number = db.Column(db.Integer)

@hybrid_property
def test(self):
    return self.number + 1

# Required for administrative interface
def __unicode__(self):
    return self.username

class UserAdmin(sqlamodel.ModelView): list_columns = ('number', 'test')

You can also make it sortable:

class UserAdmin(sqlamodel.ModelView): list_columns = ('number', 'test') sortable_columns = (('test', 'number'),)

— Reply to this email directly or view it on GitHubhttps://github.com/mrjoes/flask-admin/issues/78#issuecomment-15137737 .

paablux commented 4 years ago

Can this be displayed when using a inline_model?