More field tests + JSONField

[grigi]

I did some more Field tests, and ported JSONField to be more generic.

Items of work:

• Test all data fields
• Test that nullable works
• Test behaviour specs of fields (DecimalField/DateTimeField)
• Generic IntegrityError
• DecimalField → fix quantization misbehaviour for SQLite
• DB-executor level to_db_value overrides (for SQLite)
• JSONField is now generic
• Update docs re supported PostgreSQL versions.
• Added queryset.get() method (currently just a shortcut for filter.first)
• values() and values_list() now converts field values to python types

Bugs uncovered/fixed:

• FloatField was never mapped to DB type
• SQLite doesn't respect Decimal quantization (easily emulatable)
• SQLite requires BOOL to be emulated as integer. So 0, 1 & null
• PostgreSQL 9.4 needed for JSONB type (updated travis and should update docs)
• Fixed an assert on values_list() to not REQUIRE flat=True if only one parameter is provided

Out of scope, because this PR is getting quite big:

• Make fields overrideable on a per-db-driver level, so that we can contain all related logic in one place
• Fields Schema generation refactoring

[coveralls]

Pull Request Test Coverage Report for Build 26

• 84 of 85 (98.82%) changed or added relevant lines in 10 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+4.3%) to 86.57%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tortoise/queryset.py	17	18	94.44%

Totals
Change from base Build 12:	4.3%
Covered Lines:	1501
Relevant Lines:	1692

---

💛 - Coveralls

[grigi]

@Zeliboba5 Please review this.

[abondar]

I think this should stay, I came up with the bug that we currently having on values and values_list - they don't serialise results with to_python_value. I came up with such test to demonstrate it:

    async def test_json_values(self):
        data = {'some': ['text', 3]}
        obj0 = await testmodels.JSONFields.create(data=data)
        values = await testmodels.JSONFields.filter(
            id=obj0.id,
        ).values('data')
        self.assertDictEqual(values[0]['data'], data)    # fails because values[0]['data'] is string

Could you please return this assertion of is None and fix values and values_list executing to serialise results

[grigi]

The to_python_value methods are only called from tortoise/models.py L333
and there it is after a guard like so:

 if not isinstance(value, field_object.type) and value is not None:
    setattr(self, key, field_object.to_python_value(value))

So that logic is duplicate.
Adding that code I took away back doesn't resolve it.

What I did wrong there is defining the type of the JSONField, as only (dict, list) whereas JSON supports more base types. The problem comes in with it being a str that is how it knows it should serialise/deserialise. I feel to solve this properly, instead of a "to_db_value" and a "to_python_value" we should change it to be a "to_db_value" and "from_db_value" (in principal).
But the current system is better ito lazy evaluation...

Another short-term possibility is to state that JSONField only works for dicts/lists as the root element?

[grigi]

Oh, I missed your point. It is a good one, and I should add _values and _values_list tests for all fields.
We might have to consider using to_python_value for that, as date/datetime would also cause issues for SQLite...

What I said about it assuming a 'str' is the serialised json object is still valid for that use case.

[abondar]

Such way to implement custom behaviour for fields in drivers seems quite hacky and not flexible enough, because it only allows to override already existing methods.
How about we do it this way:

• In module of driver you implement custom fields inheriting from their basic parent (Like PostgresJSONField(fields.JSONField):
• You create a mapping of {<Base class>: <Driver custom class>} somewhere in module
• On Tortoise.init() you patch models based on their driver class replacing original class with a class from driver app

There is one more tricky moment with this strategy - different drivers can have different name for field type in db. To have better control over it - fields themselfs should have method like get_db_type() which will return their type definition. This way we can remove FIELD_TYPE_MAP from SchemaGenerator and users will be able to define custom fields with custom db type. This way we can also remove hardcode like this from SchemaGenerator:

if isinstance(field_object, fields.DecimalField):
    field_type = field_type.format(field_object.max_digits, field_object.decimal_places)
elif isinstance(field_object, fields.CharField):
    field_type = field_type.format(field_object.max_length)

and replace it with graceful:

    def get_db_type(self):
        return 'DECIMAL({},{})'.format(self.max_digits, self.decimal_places)

[grigi]

Yeah, I was thinking it would be better to have the DB driver replace the Field instances with their own versions on create. Then all the custom logic could be contained.
The issue is there is a LOT of refactoring that would be required for that. So I wasn't sure what to do with it.

If we want to clean up the code structure, there is a lot of annoyances that are going to get in the way:

1. The logic is currently spread out a bit already
2. There are quite a few circular imports
3. There is a fair amount of duplication.
   I should probably do some more investigation around this.

[grigi]

I'll add more tests first, then try and fix the broken ones, then start on the refactor. I will need some time before I can be working on this again (possibly only the weekend).

[grigi]

@Zeliboba5 Could you pore over commit 88a2b82 as it is easily the most invasive change I have done so far.
I tried to structure it in a way that would minimise excessive overhead, but we will have to profile it to actually see if it is a real opportunity for optimisation.
(e.g. only one field type needs wrapping for asyncpg, whereas five for sqlite. We could identify this isn't a needed step and skip it in those cases.)

[grigi]

@Zeliboba5 I also think we should leave the field refactoring out for the next PR, as this PR is already doing 3 different things:

• Fields tests (and fixes)
• JSONField
• values() and values_list() now converts field values to python types

[abondar]

88a2b82 seems perfectly sane to me.
Regarding fields refactoring - it really seems to be out of the scope of this PR, which is already quite big.
I will publish this as 0.9.2 version.
Thanks for your great work 👍