Entities have code preventing mocking and therefore aren't test-friendly #141

Closed
nbrochu opened this Issue Aug 19, 2015 · 4 comments

Projects

None yet

3 participants

@nbrochu
nbrochu commented Aug 19, 2015

From core.py:

class EntityMeta(type):
    def __setattr__(entity, name, val):
        if name.startswith('_') and name.endswith('_'):
            type.__setattr__(entity, name, val)
        else: throw(NotImplementedError)

Any specific reason for the redefinition of setattr here? When I got started with Pony a couple of months back, I spent some time determining if I could add instance and class methods directly in the classes that inherit from db.Entity. I saw nothing in the documentation indicating that it was a bad idea to do so, in fact it seems like the recommended thing to do in OOP.

All my Entity classes ended up with stuff similar to this:

class Country(db.Entity):
    _table_ = "countries"

    id = PrimaryKey(int, sql_type="bigserial", auto=True)
    # ...

    def as_json(self):
        pass

    def index_in_elasticsearch(self):
        pass

    @classmethod
    def search(cls, query):
        pass

Everything works as intended. Until I start unit testing.

Let's say that I want to test that index_in_elasticsearch is called 3 times with different entities. The normal thing to do would be to mock that instance method and assert using the mocked object. The mocking library in the Python stdlib uses setattr and because of that redefinition in EntityMeta, I get NotImplementedError.

I can get around it easily by doing:

def set_attr_func(entity, name, val):
        type.__setattr__(entity, name, val)

mocker.patch("pony.orm.core.EntityMeta.__setattr__", new=set_attr_func)

but it feels dirty and I have no idea if I'm breaking something doing so.

Is this a legitimate issue? Are we not supposed to add functionality to the Entity classes? Feels like it would be a lot of boilerplate to write a proxy class (i.e. CountryModel) to have testable functionality. If that's the good way of doing it, the documentation should be clearer.

@kozlovsky
Contributor

The reason why we restrict attribute assignment to entity classes is that initially we had an idea to use such assignment to describe migrations, like:

Country.population = Optional(int)  # adding new attribute
Country.languages = Set("Language") # adding new relation
del Country.language # drop obsolete column

Allowing users to assign custom attributes to entity classes could prevent using such assignment for migrations because of backward compatibility problems.

Probably it was not too good idea to express migrations this way, so probably we will remove this restriction in the near future.

In the mean time, you can solve your problem using mixin classes. Instead of putting custom methods directly to the entity definition, you can define them in a separate mixin class and inherit entity class from that mixin:

class CountryMixin(object):
    def as_json(self):
        pass

    def index_in_elasticsearch(self):
        pass

    @classmethod
        def search(cls, query):
            pass

class Country(db.Entity, CountryMixin):
     _table_ = "countries"

    id = PrimaryKey(int, sql_type="bigserial", auto=True)
    # ...

Then you can patch the mixin class instead of the entity class. Since the entity class inherited all mixin methods it will use patched versions of these method.

Another potential benefit of using mixins can be found if a developer uses our online diagram editor: http://editor.ponyorm.com. The editor can generate entity definitions automatically. But if a programmer added some custom methods directly to the entity definition these methods will be overwritten if some changes was made to the graphical diagram. Using mixin it will be possible to separate entity definition to two different files. One file will contain autogenerated entity definitions, and another file will contain mixins with additional methods. This way it will be possible to save new autogenerated version of entity definition without losing all custom methods.

Tell me what do you think, can using mixins be appropriate solution in your case?

@nbrochu
nbrochu commented Aug 20, 2015

Seems like that would be a good and logical pattern. I'll give it a test spin tomorrow.

Would it be wise to suggest this pattern in the documentation? I'm guessing I'm not the only one that did this.

Thanks!

@amalashkevich
Member

Thanks for the question! We've just added the description of this pattern to our documentation http://doc.ponyorm.com/entities.html#adding-custom-methods-to-entities

Please let us know if you have other questions.

@nbrochu
nbrochu commented Aug 21, 2015

Perfect! I tried it with a smaller model and it works and is clean. A good addition to the documentation!

Thanks.

@nbrochu nbrochu closed this Aug 21, 2015
@kozlovsky kozlovsky added a commit that referenced this issue Jan 11, 2016
@kozlovsky kozlovsky Pony ORM Release 0.6.2 (2015-01-11)
The documentation was moved from this repo to a separate one at https://github.com/ponyorm/pony-doc
The compiled version can be found at https://docs.ponyorm.com

# New features

* Python 3.5 support
* #132, #145: raw_sql() function was added
* #126: Ability to use @db_session with generator functions
* #116: Add support to select by UUID
* Ability to get string SQL statement using the Query.get_sql() method
* New function delete(gen) and Query.delete(bulk=False)
* Now it is possible to override Entity.__init__() and declare custom entity methods

# Backward incompatible changes

* Normalizing table names for symmetric relationships
* Autostrip - automatically remove leading and trailing characters

# Bugfixes

* #87: Pony fails with pymysql installed as MySQLdb
* #118: Pony should reconnect if previous connection was created before process was forked
* #121: Unable to update value of unique attribute
* #122: AssertionError when changing part of a composite key
* #127: a workaround for incorrect pysqlite locking behavior
* #136: Cascade delete does not work correctly for one-to-one relationships
* #141, #143: remove restriction on adding new methods to entities
* #142: Entity.select_random() AssertionError
* #147: Add 'atom_expr' symbol handling for Python 3.5 grammar
ab09f64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment