Issue: BinaryField/BlobField/UUIDField is broken with MySQL
Author: foxx
State: closed
Created: 2016-01-14 16:28:20

Looks like both `BlobField` and `BinaryField` in MySQL are actually broken, because MySQL expects a `max_length`, and thus all values are being truncated down to 1 byte.

The most obvious fix seems to be;

```
class BinaryField(BlobField):
    db_field = 'varbinary'

class FixedBinaryField(CharField):
    db_field = 'binary'    
```

The test should then probably be updated to include a `data` value comparison, and then add the appropriate database mappings after.

Submitting PR shortly


Just looking into this a bit deeper, it seems that `BlobField` is actually broken in MySQL as well, as data was being truncated and not detected by tests, due to [this](https://github.com/coleifer/peewee/blob/master/playhouse/tests/test_fields.py#L320). As you can see, the test was referencing `blob` rather than `res`.

I'm also seeing a /lot/ of warnings locally;

```
/vagrant/peewee/peewee.py:3480: Warning: Unknown table 'testmodelb'
  cursor.execute(sql, params or ())
/vagrant/peewee/peewee.py:3480: Warning: Unknown table 'testmodela'
  cursor.execute(sql, params or ())
/vagrant/peewee/peewee.py:3480: Warning: Unknown table 'computer'
  cursor.execute(sql, params or ())
```

PR coming shortly, not sure if all the tests are working because getting the test suite to run locally for postgres seems to be troublesome, seeing lots of this;

```
Traceback (most recent call last):
  File "/vagrant/peewee/playhouse/tests/base.py", line 284, in setUp
    test_db.drop_tables(self.requires, True)
  File "/vagrant/peewee/peewee.py", line 3597, in drop_tables
    drop_model_tables(models, fail_silently=safe, cascade=cascade)
  File "/vagrant/peewee/peewee.py", line 4901, in drop_model_tables
    m.drop_table(**drop_table_kwargs)
  File "/vagrant/peewee/peewee.py", line 4633, in drop_table
    cls._meta.database.drop_table(cls, fail_silently, cascade)
  File "/vagrant/peewee/peewee.py", line 3594, in drop_table
    model_class, fail_silently, cascade))
  File "/vagrant/peewee/peewee.py", line 3487, in execute_sql
    self.commit()
  File "/vagrant/peewee/peewee.py", line 3311, in __exit__
    reraise(new_type, new_type(*exc_args), traceback)
  File "/vagrant/peewee/peewee.py", line 3480, in execute_sql
    cursor.execute(sql, params or ())
InternalError: current transaction is aborted, commands ignored until end of transaction block
```

It might be worth adding a `Vagrantfile` with a provisioner so that people can contribute and run the test suite without any hassle of setting up a container environment themselves, I'll raise another issue for this though.

PR coming now


Alright I've narrowed it down, the concept of `BinaryField` in MySQL and Postgres is very different. `BINARY` and `VARBINARY` require a `max_length` where as Postgres `BYTEA` doesn't have such an option.

These field types are fundamentally incompatible between databases, and therefore I have modified the tests to only support `BinaryField` and `FixedBinaryField` with MySQL. 

An alternative solution is to add a conditional into `get_modifiers()` which checks `isinstance(self._meta.database, MySQLDatabase)` and make it not return a modifier if the database doesn't match MySQL. I wasn't sure if this was a violation of Peewee design pattern though.

@coleifer Thoughts?


Looks like changing `get_modifiers()` is working pretty well, updated PR to follow.


After smashing my head against a brick wall for the last few hours, I've finally come to the conclusion that my PR for `BinaryField` and `UUIDField` were completely flawed.

Using `get_modifiers()` is not really an option because it results in different field behaviour between each database. For example, postgres would allow an unlimited data value size, where as MySQL would not.

I would argue that the usage of `BinaryField` and `UUIDField` in MySQL are so limited, and in fact, almost no one in their right mind would use `UUIDField` in MySQL using the current `VARCHAR(255)` type, so all we've done is add unnecessary complexity to the core.

Therefore my next proposal would be to create `playhouse.mysql` and move those two fields into that module. If that isn't accepted, then I would probably vote to have `BinaryField` removed entirely, because in their current implementation they just cause confusion and problems. I'd also vote to have `VARCHAR(255)` removed from `UUIDField`

I've left my PR open anyway so you can see how I came to this conclusion.

PS) Apologies for causing so much wasted time with this, I should have used these two fields in dev and prod for a few months before submitting ;/


Honestly, I haven't really read the comments... I'm just looking at the code and I don't see how there's a problem unless this test is just completely broken:

``` python
    @skip_test_unless(lambda: isinstance(test_db, MySQLDatabase))
    def test_blob_and_binary_field_mysql(self):
        def inner(cls):
            data = bytes(bytearray(range(256)))
            blob = cls.create(data=Param(data))
            res = cls.get(cls.id == blob.id)
            self.assertEqual(blob.data, data)

        inner(BlobModel)
```

It appears to be comparing the saved object's `data` field with a 256-byte string.


Oh shit, ok now I read your comments haha ...


Tbh the easiest fix for all of this is to just rip out my `UUIDField` and `BinaryField` PRs. I'll be adding a better abstraction into `peewee_extras` for both of these, and if they are stable after a couple of months prod usage, then we could re-assess putting them into playhouse? If it helps I can send a PR for this.


I killed `BinaryField` and made `UUIDField` a VARCHAR(40), which seemed more appropriate but perhaps you have a better idea for that one. I think that putting them into a `playhouse.mysql_ext` module is a good solution. I have neither the inclination nor the perseverance to try and figure out what is going on with MySQL, so shoving things into a module where they can't hurt anybody is fine.


Completely agreed, and in hindsight I should have done this from the beginning. Since our last discussion I've actually moved over to Postgres, after suffering horrible problems with JSON columns in MariaDB. Once I've got `peewee-extras` stable for a few months I will make an effort to submit a PR for `playhouse.mysql_ext` at a later date, as I hate to leave a job unfinished. Apologies again for all the time wasting, I owe you a few hours of contributor time!


> suffering horrible problems with JSON columns in MariaDB. 

When you have a few minutes to spare, I'd be curious what went wrong. Or what didn't work like you expected there.


So basically MariaDB supports JSON via [`column_json`](https://mariadb.com/kb/en/mariadb/column_json/) at query time, rather than a data type in schema, which visually looks quite strange. But this gets even stranger when using [dynamic columns](https://mariadb.com/kb/en/mariadb/dynamic-columns-in-mariadb-10/), to the point that the syntax is almost incomprehensible. There are some usage examples [here](http://stackoverflow.com/questions/23087105/mariadb-dynamic-columns-json), and trying to make these work with an ORM would be painful at best.

From what I can tell, `column_json` is converting the data to JSON on each blob read, which from a performance standpoint would most likely be significantly worse when compared to Postgres, which has native JSON type support. I believe dynamic columns are supposed to improve this, but the syntax is poor.

Then there is the lack of native UUID type, it can be argued that these could be easily stored as BINARY() but this makes for harder readability for debugging, especially when inspecting pcaps for debugging purposes.

Overall MariaDB has a lot of improvements over MySQL, and I have a huge amount of respect for the work that Monty and the team are doing, but they have really dropped the ball with UUID and JSON support, sadly.

I've ran out of time to do any more work on it this week, but I'll be following up with a blog post showing the performance of UUID field in Postgres, as I'm curious to know whether they have fixed the slow inserts problem, or whether `OrderedUUIDField` would still be needed. (based on that percona [article](https://www.percona.com/blog/2014/12/19/store-uuid-optimized-way/) I mentioned a few months back)


Awesome, thanks for the info! Please send me a link to your blog, love to
take a read.
On Jan 14, 2016 5:27 PM, "Cal Leeming" notifications@github.com wrote:

> So basically MariaDB supports JSON via column_json
> https://mariadb.com/kb/en/mariadb/column_json/ at query time, rather
> than a data type in schema, which visually looks quite strange. But this
> gets even stranger when using dynamic columns
> https://mariadb.com/kb/en/mariadb/dynamic-columns-in-mariadb-10/, to
> the point that the syntax is almost incomprehensible. There are some usage
> examples here
> http://stackoverflow.com/questions/23087105/mariadb-dynamic-columns-json,
> and trying to make these work with an ORM would be painful at best.
> 
> From what I can tell, column_json is converting the data to JSON on each
> blob read, which from a performance standpoint would most likely be
> significantly worse when compared to Postgres, which has native JSON type
> support. I believe dynamic columns are supposed to improve this, but the
> syntax is poor.
> 
> Then there is the lack of native UUID types, it can be argued that these
> could be easily stored as BINARY() but this makes for harder readability
> for debugging, especially when inspecting pcaps for debugging purposes.
> 
> Overall MariaDB has a lot of improvements over MySQL, and I have a huge
> amount of respect for the work that Monty and the team are doing, but they
> have really dropped the ball with UUID and JSON support, sadly.
> 
> I've ran out of time to do any more work on it this week, but I'll be
> following up with a blog post showing the performance of UUID field in
> Postgres, as I'm curious to know whether they have fixed the slow inserts
> problem, or whether OrderedUUIDField would still be needed. (based on
> that percona article
> https://www.percona.com/blog/2014/12/19/store-uuid-optimized-way/ I
> mentioned a few months back)
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/coleifer/peewee/issues/818#issuecomment-171815713.