Tortoise.init() refactoring and transactions module #29

abondar · 2018-07-17T11:45:53Z

#14
This seems to be biggest change that occurred to tortoise and first one to bring so much backward incompatible changes.

Tortoise.init() is now coroutine that handles all connections and models discovery.
New transactions that finally work without explicit connection passing and also can be used for easily changing db when working with models.

This is still work in progress, I want to make few more changes, update all documentation and may be some more tests.

coveralls · 2018-07-17T13:04:40Z

Pull Request Test Coverage Report for Build 108

354 of 582 (60.82%) changed or added relevant lines in 21 files are covered.
105 unchanged lines in 5 files lost coverage.
Overall coverage increased (+2.2%) to 90.653%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tortoise/transactions.py	33	34	97.06%
tortoise/backends/asyncpg/init.py	0	2	0.0%
tortoise/queryset.py	21	23	91.3%
tortoise/backends/mysql/init.py	0	2	0.0%
tortoise/contrib/test/init.py	44	46	95.65%
tortoise/backends/base/client.py	8	11	72.73%
tortoise/init.py	118	121	97.52%
tortoise/models.py	44	48	91.67%
tortoise/backends/mysql/schema_generator.py	0	13	0.0%
tortoise/backends/mysql/executor.py	0	29	0.0%

Files with Coverage Reduction	New Missed Lines	%
tortoise/backends/base/executor.py	1	95.56%
tortoise/backends/base/client.py	6	90.74%
tortoise/backends/asyncpg/schema_generator.py	8	100.0%
tortoise/backends/asyncpg/executor.py	13	100.0%
tortoise/backends/asyncpg/client.py	77	83.01%

Totals
Change from base Build 63:	2.2%
Covered Lines:	1986
Relevant Lines:	2134

💛 - Coveralls

grigi · 2018-07-17T13:55:06Z

examples/aggregation.py

-    await generate_schema(client)
-
-    tournament = Tournament(name='New Tournament')
+    await Tortoise.init(config_file='config.json')


Could we please have this split up?
I mean Tortoise.init() should instead recieve configuration in a dict, and for cases where we want to have support of config files or enviromment variables should be done as a layer above that.

This will make it easier to wrap the ORM in another framework.

Well - it already receives config as dict as main method of init - look for postgres example.
config_file is just alternative option for those who use tortoise directly

grigi · 2018-07-17T17:18:07Z

examples/postgres.py

+                    'port': '54325',
+                    'user': 'tortoise',
+                    'password': 'qwerty123',
+                    'database': 'test',


Is it possible to use the db_url stuff I did?
It is a lot more concise, and since we changed over to DB-URLS at work it really made configuration much simpler, as e.g. with Docker you can easily inject environment variables, so we use that for configuration.

Yes, it's possible. I refactored db_url to config_generator - you can see how config generated for it in test_db_url

I feel we should make the examples use that, as it is more concise.
Or just document both, a "shorthand" that is fine for most use cases, and an "advanced"?

grigi · 2018-07-17T17:20:21Z

examples/transactions.py

+    saved_event = await Event.filter(name='Updated name').first()
+    print(saved_event)
+
+    @atomic()


YAY!
I had lots of reservations about how transactions worked, as they were very non-intuive.

Is it possible to do with atomic:?

You can do it with async with in_transaction(): without calling it from connection. I separated methods because I am not sure it's intuitive to have function overloaded with two separate effects.

It makes sense to me to use the same word.
The decorator wraps the whole function in a transaction.
The with statement wraps the whole block in a transaction.

A different question, I wanted to do a test case where I do the following (but could not get it working with the previous transaction system):

Enter transaction

make change 1

Test change 1

Commit

Test change 1

make change 2

test change 2

rollback

test change 2 fail

Could you add such a test case (and any variations you can think of)?

grigi

Looking good so far

grigi · 2018-07-17T17:24:02Z

tortoise/__init__.py

                    )

                    related_model._meta.backward_fk_fields.add(backward_relation_name)
                    related_model._meta.fetch_fields.add(backward_relation_name)
-                    related_model._meta.fields_map[backward_relation_name] = fk_relation
+                    related_model._meta.fields_map[backward_relation_name] = relation


The reason I split relation into fk_relation and m2m_relation was because these two variables are actually different types, and the type checker got really badly confused.

Oh, it's just my bad, I merged it badly today, will revert back

grigi · 2018-07-17T17:40:51Z

tortoise/backends/base/config_generator.py



-def expand_db_url(db_url: str, testing: bool = False) -> dict:
+def generate_config(


Not sure I like this.
Please try to keep to the single responsibility principal where you can.
e.g.
One function to resolve the {engine-credentials} dict, and another to generate the config per app.
Mostly to keep it easier to test, but also minimises function complexity, so makes it less invasive when one inevitably wants to refactor something.

grigi · 2018-07-17T17:42:19Z

tortoise/contrib/test/__init__.py

-    An asyncio capable test class that will ensure that an partially isolated test db
-    is available for each test.
+    An asyncio capable test class that will ensure that each test will be run at
+    separate transaction that will rollback on finish.


grigi · 2018-07-17T18:13:14Z

tox.ini

@@ -1,5 +1,5 @@
 [tox]
-envlist = py{35,36}-{sqlite,postgres}
+envlist = py{36,37}-{sqlite,postgres}


You can leave it as py{35,36,37}... and then just set:
skip_missing_interpreters = True
To skip that which you don't have installed.

…g style

…ds (ex: char in unittest)

grigi · 2018-07-20T10:37:42Z

I have been playing with the test runner, there is a lot of things not quite right for the test runner.

I have added initialiser and finaliser so that Testcase cleans up after itself, and does the migrations exactly once per test process.
There is issues with transactions on SQLite, and also the _drop_databases() command doesn't seem to actually drop anything?

Anyways I'm just stating that I'm refactoring the test suite again.

Feature/python3.7

If a pk isn't defined, we create a pk called id for that model. Also added Model method tests.

Default model pk: id = fields.IntField(pk=True)

grigi · 2018-07-26T09:29:47Z

I have been building tests for init, and actually now have feedback to give.
There is some inconsistencies in how it appears to work.
I'll get to it the weekend, and it may be me missing some minor point.

Update 1: * Fixed resource leak in init_tests.py Update 2: * Restored bad merge for fk_relation and m2m_relation in Tortoise.init() * More typing annotations * Tortoise.init() directly supports generate_config parameters * Break up generate_config to use generate_db_url again * Fix Model.get_or_create to return actual instance, not awaitable * get_backward_fk_filters was never called * Changed generate_config to take a dict of app: [model modules] * Changed generate_config to call DB config "default" by default. Update 3: * Fix inconsistency in shorthand format

Added more Tortoise.init() tests

grigi · 2018-07-31T07:46:00Z

@Zeliboba5 At this stage I'm happy with this PR, and I'm giving the 👍 to merge it to master.
Then we can look at #37 a bit more leisurely.

grigi · 2018-07-31T07:46:18Z

And great working with you 🙇‍♂️

grigi

👍

abondar · 2018-07-31T15:38:40Z

@grigi Do you think we should do anything special about this PR, considering it will be backward incompatible? I don't want to publish it as 1.0 already, so may be we should just make some special mark in changelog?

grigi · 2018-07-31T18:14:44Z

@Zeliboba5: v0.10.0 + a mention that it has breaking changes re init should be enough, I think.
Maybe a short little blurb with examples in the CHANGELOG.rst file?

…ise-orm into feature/mysql

MySQL support

abondar · 2018-08-01T09:41:42Z

I am really glad that we managed to make this release.
@grigi @etzelwu Thanks for great work 👍

abondar added 2 commits July 17, 2018 14:37

Tortoise.init() refactoring and transactions module

d6472fa

Linter fixes

3e69429

grigi reviewed Jul 17, 2018

View reviewed changes

abondar added 2 commits July 17, 2018 17:20

Fixed postgres tests errors and 3.5 compatibility

04e279d

Another linter fixes

1149a21

grigi reviewed Jul 17, 2018

View reviewed changes

This was referenced Jul 17, 2018

Discussion: v1.0.0 target #19

Open

Feature/python3.7 #28

Merged

etzelwu and others added 18 commits July 18, 2018 12:40

Intergrated with MySQL.

2ad7cb2

Add little more mysql functions.

3e78635

Edit mysql/schema_generator.py

9012204

Add 'from tortoise.utils import generate_schema'

e4412af

Let sql templates become class attribute and add some functions.

9d33ad3

Add basic functions (insert, select)

db76ee3

Add mysql/client.py/(db_create, db_delete) functions and revise codin…

46ca1b4

…g style

Add mysql/client.py/(in_transaction) function and TransactionWrapper.

46da565

Add mysql into configuration

6a21b58

Fix mysql url and add mysql testing into tests/test_db_url

f788f52

Fix test_db_url and mysql/client

775763b

Bump deps, set Py3.7 for CI

11cac82

try different dist

945ad57

flail travis, flail

caa1557

Make build allow failiures for 3.7-sqlite

c29a3cf

Wrap indentifiers with backticks to prevent conflicts of reserved wor…

51fc4ac

…ds (ex: char in unittest)

Spelling fixes and more type fixes

4fde48e

Insert dafault value CURRENT_TIMESTAMP into DatetimeField template

ff10a21

abondar and others added 2 commits July 25, 2018 12:01

Merge pull request #28 from grigi/feature/python3.7

5612e86

Feature/python3.7

Default model pk: id = fields.IntField(pk=True)

51b7841

If a pk isn't defined, we create a pk called id for that model. Also added Model method tests.

grigi mentioned this pull request Jul 25, 2018

Default model pk: id = fields.IntField(pk=True) #34

Merged

grigi and others added 3 commits July 25, 2018 16:33

Documentation updates

abc4384

Add mocks of optional libraries for autodoc

8490ab9

Merge pull request #34 from grigi/feature/model_tests

bae782c

Default model pk: id = fields.IntField(pk=True)

grigi and others added 6 commits July 29, 2018 07:49

Documentation re init()

6a76e35

Merge branch 'feature/init_tests' into feature/mysql

4825304

Added MySQL/MariaDB to docs

1aff9b4

Updated changelog/contrib

b05044e

Merge pull request #35 from grigi/feature/init_tests

54b5910

Added more Tortoise.init() tests

grigi approved these changes Jul 31, 2018

View reviewed changes

grigi and others added 2 commits July 31, 2018 10:02

Changes as per review

adb1b2e

Overriding filter funcs in executor class for different clients

39be780

grigi and others added 7 commits July 31, 2018 20:18

Merge branch 'feature/init-refactoring' of github.com:Zeliboba5/torto…

29434d2

…ise-orm into feature/mysql

Caution about breaking change with init

07ef73a

Implemented filter function override for MySQL/MariaDB

a917347

Merge pull request #37 from grigi/feature/mysql

cd0ca34

MySQL support

Bumped version to 0.10.0

13d05c2

Updated README for new init method

98cbe13

Fixed typo in docs

1fdfe5e

abondar merged commit 7a3fa6d into master Aug 1, 2018

abondar deleted the feature/init-refactoring branch November 23, 2018 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tortoise.init() refactoring and transactions module #29

Tortoise.init() refactoring and transactions module #29

abondar commented Jul 17, 2018 •

edited

coveralls commented Jul 17, 2018 •

edited

grigi Jul 17, 2018

abondar Jul 17, 2018

grigi Jul 17, 2018

grigi Jul 17, 2018

abondar Jul 17, 2018 •

edited

grigi Jul 17, 2018

grigi Jul 17, 2018

abondar Jul 17, 2018

grigi Jul 17, 2018

grigi left a comment

grigi Jul 17, 2018

abondar Jul 17, 2018

grigi Jul 17, 2018

grigi Jul 17, 2018

grigi Jul 17, 2018

grigi commented Jul 20, 2018

grigi commented Jul 26, 2018

grigi commented Jul 31, 2018

grigi commented Jul 31, 2018

grigi left a comment

abondar commented Jul 31, 2018

grigi commented Jul 31, 2018

abondar commented Aug 1, 2018



		def expand_db_url(db_url: str, testing: bool = False) -> dict:
		def generate_config(

Tortoise.init() refactoring and transactions module #29

Tortoise.init() refactoring and transactions module #29

Conversation

abondar commented Jul 17, 2018 • edited

coveralls commented Jul 17, 2018 • edited

Pull Request Test Coverage Report for Build 108

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abondar Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grigi commented Jul 20, 2018

grigi commented Jul 26, 2018

grigi commented Jul 31, 2018

grigi commented Jul 31, 2018

grigi left a comment

Choose a reason for hiding this comment

abondar commented Jul 31, 2018

grigi commented Jul 31, 2018

abondar commented Aug 1, 2018

abondar commented Jul 17, 2018 •

edited

coveralls commented Jul 17, 2018 •

edited

abondar Jul 17, 2018 •

edited