api: native crud module support by GRISHNOV · Pull Request #264 · tarantool/tarantool-python

GRISHNOV · 2022-11-15T20:55:26Z

Adds native api support for crud module to use it from a connection object.

Below there are examples of using the api.

>>>importtarantool>>>fromtarantool.errorimportCrudModuleError, CrudModuleManyError, DatabaseError>>>conn=tarantool.Connection(host='localhost',port=3301)
>>>conn.crud_conn.crud_count( conn.crud_insert( conn.crud_insert_object_many( conn.crud_min( conn.crud_replace_object( conn.crud_stats( conn.crud_unflatten_rows( conn.crud_upsert_many( conn.crud_delete( conn.crud_insert_many( conn.crud_len( conn.crud_replace( conn.crud_replace_object_many( conn.crud_storage_info( conn.crud_update( conn.crud_upsert_object( conn.crud_get( conn.crud_insert_object( conn.crud_max( conn.crud_replace_many( conn.crud_select( conn.crud_truncate( conn.crud_upsert( conn.crud_upsert_object_many(

Inserting data via crud:

# Insert without exception:>>>res=conn.crud_insert('tester', (3500,300,'Rob'))
>>>res<tarantool.crud.CrudResultobjectat0x11a56e320>>>>res.
res.metadatares.rows>>>res.rows
[[3500, 300, 'Rob']]
>>>res.metadata
[{'name': 'id', 'type': 'unsigned'}, {'name': 'bucket_id', 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}]
# Insert with exception (duplicate key exists):>>>try:
... res=conn.crud_insert('tester', (3500,300,'Rob'))
... exceptCrudModuleErrorase:
... exc_crud=e
... >>>exc_crudCrudModuleError(0, 'Failed to insert: Duplicate key exists in unique index "primary_index" in space "tester" with old tuple - [3500, 300, "Rob"] and new tuple - [3500, 300, "Rob"]')
>>>exc_crud.extra_info_error<tarantool.crud.CrudErrorobjectat0x10a276950>>>>exc_crud.extra_info_error.
exc_crud.extra_info_error.class_nameexc_crud.extra_info_error.errexc_crud.extra_info_error.fileexc_crud.extra_info_error.lineexc_crud.extra_info_error.str>>>exc_crud.extra_info_error.class_name'InsertError'>>>exc_crud.extra_info_error.str'InsertError: Failed to insert: Duplicate key exists in unique index "primary_index" in space "tester" with old tuple - [3500, 300, "Rob"] and new tuple - [3500, 300, "Rob"]'# In case of batch operation (*_many), CrudModuleManyError exception contains both result and errors (if there is a problem with at least one row).>>>try:
... res=conn.crud_insert_object_many('tester', ({'id':3,'bucket_id':100,'name':'Ann'}, {'id':4,'bucket_id':100,'name':'Sam'}), {'timeout':100, 'rollback_on_error':False})
... exceptCrudModuleManyErrorase:
... exc_crud=e
... >>>exc_crudCrudModuleManyError(0, 'Got multiple errors, see errors_list')
>>>exc_crud.success_list# some of the rows were inserted.<tarantool.crud.CrudResultobjectat0x11a56f310>>>>exc_crud.success_list.rows
[[1, 100, 'Bob'], [2, 100, 'Rob']]
>>>exc_crud.errors_list# some of the rows were not inserted.
[<tarantool.crud.CrudErrorobjectat0x11a56e9e0>, <tarantool.crud.CrudErrorobjectat0x11a56f490>]
>>>exc_crud.errors_list[0].str'CallError: Failed for 037adb3a-b9e3-4f78-a6d1-9f0cdb6cbefc: Function returned an error: Duplicate key exists in unique index "primary_index" in space "tester" with old tuple - [3500, 300, "Rob"] and new tuple - [3500, 100, "Mike"]'>>>exc_crud.errors_list[1].str'InsertManyError: Failed to flatten object: FlattenError: Object is specified in bad format: FlattenError: Unknown field "second_name" is specified'# If there are no problems with any rows, the entire response will be contained in the res variable.>>>res=conn.crud_insert_object_many('tester', ({'id':3,'bucket_id':100,'name':'Ann'}, {'id':4,'bucket_id':100,'name':'Sam'}), {'timeout':100, 'rollback_on_error':False})
>>>res.rows
[[3, 100, 'Ann'], [4, 100, 'Sam']]

Crud not found on the router:

>>>try:
... res=conn.crud_insert('tester', (22221,300,'Rob'))
... exceptDatabaseErrorase:
... exc_db=e
... >>>exc_dbDatabaseError(33, "Ensure that you're calling crud.router and user has sufficient grants")
>>>exc_db.extra_infoBoxError(type='ClientError', file='/tmp/tarantool-20221003-6335-edruh3/tarantool-2.10.3/src/box/lua/call.c', line=112, message="Procedure 'crud.insert' is not defined", errno=0, errcode=33, fields=None, prev=None)

Select and unflatten_rows via crud:

>>>try:
... res=conn.crud_select('tester', {}, {'first':2})
... exceptCrudModuleErrorase:
... exc_crud=e
... >>>res<tarantool.crud.CrudResultobjectat0x10a276d10>>>>res.
res.metadatares.rows>>>res.rows
[[1, 100, 'Mike'], [2, 100, 'Mike']]
>>>res.metadata
[{'name': 'id', 'type': 'unsigned'}, {'name': 'bucket_id', 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}]
>>>r=conn.crud_unflatten_rows(res.rows, res.metadata)
>>>r
[{'id': 1, 'bucket_id': 100, 'name': 'Mike'}, {'id': 2, 'bucket_id': 100, 'name': 'Mike'}]

Truncate and len via crud:

>>>try:
... res=conn.crud_len('tester')
... exceptCrudModuleErrorase:
... exc_crud=e>>>res26>>>try:
... res=conn.crud_truncate('tester')
... exceptCrudModuleErrorase:
... exc_crud=e>>>resTrue

Encoding=None in Connection:

>>>conn=tarantool.Connection(host='localhost',port=3301,encoding=None)
>>>try:
... res=conn.crud_insert('tester', (3500,300,'Rob'))
... exceptCrudModuleErrorase:
... exc_crud=e
... >>>res<tarantool.crud.CrudResultobjectat0x1188a7fd0>>>>res.
res.metadatares.rows>>>res.metadata
[{b'name': b'id', b'type': b'unsigned'}, {b'name': b'bucket_id', b'type': b'unsigned'}, {b'name': b'name', b'type': b'string'}]
>>>res.rows
[[3500, 300, b'Rob']]

Closes#205

DifferentialOrange

Thank you for your draft! The task it much trickier that I initially have expected. I gave it some thought (and recalled some crud behavior) and I think that we should try to rework some things. I'll summarize my comments here.

Try to get rid of class Crud.
Try to work with calls only.
Do not expose configuration handles.
Raise errors as exceptions.
Optimize utils usage.

I see that the first one is a requirement to be able to use conn.crud.method, but is such approach really widespread in Python? I'm not well-experienced myself, but I think that I have seen providing many handles from a single place more often than using complex hierarchy. Maybe it's worth to study this question a little by using some Python modules as an example (and if you still decide to use conn.crud.method, maybe you'll see some nice implementation approaches).

It is highly likely that I have forgot to mention something important in this review, so feel free to ask me any additional questions here or in personal chat.

GRISHNOV · 2022-11-22T20:32:26Z

Thank you for your feedback!

I have made most of the edits (with the exception of the pairs implementation).
If the current API is suitable, I'll move on to writing documentation and tests

DifferentialOrange · 2022-11-23T09:34:49Z

>>>exc_crud.resisNone# use only in case of *_many

I don't get this comment.

GRISHNOV · 2022-11-23T09:40:35Z

>>>exc_crud.resisNone# use only in case of *_many
I don't get this comment.

I mean, in exc_crud.res, the result can only appear when the *_many() is called (when part of the batch is successful and part is not), in other cases exc_crud.res will always be None

DifferentialOrange

Thank you! I think current API is fine and we may continue to work with it.

GRISHNOV · 2022-11-29T13:34:55Z

Thank you!
At the moment, I have implemented tests and documentation, and removed the draft status from the PR

DifferentialOrange

Thank you for the update!

See comments below. The main drawback of the current version is that tests are actually not run on CI, so I think you would be interested to fix it first and then fix everything that fails on CI.

There are some things that maybe are not covered yet, but there should be enough work for now.

DifferentialOrange

I think the only one unresolved question is errors. Every other thing seems ok.

DifferentialOrange · 2022-12-09T11:25:07Z

+ self.errs = args[1]
+ else:
+ # Sets tarantool.crud.CrudError object.
+ self.err = args[1]


Which fields do we have in CrudError? Can't we simply expose them here so it be similar to existing errors?

It depends on the method we are calling. At the moment we are just exposing the fields from the Lua implementation (args[0] is res and args[1] is err/errs Lua vars). We can implement something like switch-case for each method, but is it necessary? Or did I misunderstand the question

There is already an exception API in the connector. Receiving a different types of exceptions may be confusing, especially if they uses the same fields with different meanings.

For example
>>> c.select('myspace') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/connection.py", line 1873, in select space_name = self.schema.get_space(space_name).sid File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/schema.py", line 216, in get_space return self.fetch_space(space) File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/schema.py", line 243, in fetch_space raise SchemaError(errmsg) tarantool.error.SchemaError: There's no space with name 'myspace' >>> c.crud_select('myspace') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/connection.py", line 2550, in crud_select raise CrudModuleError(None, CrudError(crud_resp[1])) tarantool.error.CrudModuleError: (None, <tarantool.crud.CrudError object at 0x7f4864a525f0>)

Let's mimic a DatabaseError in a simple way.
What's in crud
For non-many
We have res, err, where err is a single error table from errors rock and res is nil.
For many
We have res, err, where err is an array of table errors and res is common result like in plain return.
What's in python
DatabaseError is an Exception with code, message and extra_info (the same error, but with additional fields based on provided box.error info).
How we can mix it
First case is rather simple. We can raise some error with the same code and message we've got from Tarantool. But since we've got additional info, it would be nice to provide it.
Second case is much complicated. My proposal is as follows:
Have two different classes for non-many and many errors (CrudModuleError and CrudModuleManyError).
Both should inherit DatabaseError.
For CrudModuleError, let's add the extra_info_errors attribute with the original error (DatabaseError has extra_info field with similar meaning, but different structure).
For CrudModuleManyError, let's raise error based on DatabaseError(0, "Got multiple errors, see errors_list"), store original errors list in errors_list attribute and success tuples in success_tuples attribute (I'm not insist on naming here -- it's just an example, but I think the current res, errs one is a bit confusing).
You also may define a new class for errors Lua rock errors since they always have the same structure, but this is optional -- dict is fine too.

It's not yet done: CrudModuleError inherits DatabaseError, but it never calls its __init__ (like here:
tarantool-python/tarantool/error.py
Line 262 in 1154b3d
super(NetworkError, self).__init__(0, self.message)
)

Same about CrudModuleManyError

Done, I also updated the documentation in quick-start.rst

DifferentialOrange

Sorry for the long response. I've thought that error is less trickier, I've proposed the solution.

DifferentialOrange · 2022-12-13T15:36:03Z

+ self.errs = args[1]
+ else:
+ # Sets tarantool.crud.CrudError object.
+ self.err = args[1]


There is already an exception API in the connector. Receiving a different types of exceptions may be confusing, especially if they uses the same fields with different meanings.

DifferentialOrange · 2022-12-13T15:36:16Z

+ self.errs = args[1]
+ else:
+ # Sets tarantool.crud.CrudError object.
+ self.err = args[1]


For example
>>> c.select('myspace') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/connection.py", line 1873, in select space_name = self.schema.get_space(space_name).sid File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/schema.py", line 216, in get_space return self.fetch_space(space) File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/schema.py", line 243, in fetch_space raise SchemaError(errmsg) tarantool.error.SchemaError: There's no space with name 'myspace' >>> c.crud_select('myspace') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/georgymoiseev/Development/github/tarantool/tarantool-python/tarantool/connection.py", line 2550, in crud_select raise CrudModuleError(None, CrudError(crud_resp[1])) tarantool.error.CrudModuleError: (None, <tarantool.crud.CrudError object at 0x7f4864a525f0>)

DifferentialOrange · 2022-12-13T16:35:42Z

+ info = ". Ensure that you're calling crud.router and user has sufficient grants"
+ e.message = e.message + info
+ e.extra_info.message = e.extra_info.message + info
+ e.args = (e.args[0], e.args[1] + info)


Second, let's never mess with args.
>>> class MyError(Exception): ... def __init__(self, *args): ... pass ... >>> MyError(1, 2) MyError(1, 2) >>> e = MyError(1, 2) >>> e. e.args e.with_traceback( >>> e.args (1, 2)
args exception attribute is essentially a copy of "what data was used to raise exception with".

Either you forgot to commit it or lost it while doing other reworks, but it's still in call_crud

Done, deleted

DifferentialOrange · 2022-12-20T07:48:58Z

+ .. _crud: https://github.com/tarantool/crud
+ """
+
+ def __init__(self, *args):


Since there are always two positional arguments, why use *args instead of def __init__(self, success, error):

Done, now using __init__(self, success, error):

DifferentialOrange · 2022-12-20T07:50:10Z

+ """
+
+ # Sets tarantool.crud.CrudResult object.
+ self.success = args[0]


There is no success other than nil for non-many requests, isn't it?

Done, deleted

DifferentialOrange · 2022-12-20T07:51:52Z

+ self.errs = args[1]
+ else:
+ # Sets tarantool.crud.CrudError object.
+ self.err = args[1]


It's not yet done: CrudModuleError inherits DatabaseError, but it never calls its __init__ (like here:
tarantool-python/tarantool/error.py
Line 262 in 1154b3d
super(NetworkError, self).__init__(0, self.message)
)

DifferentialOrange · 2022-12-20T07:52:16Z

+ self.errs = args[1]
+ else:
+ # Sets tarantool.crud.CrudError object.
+ self.err = args[1]


Same about CrudModuleManyError

DifferentialOrange · 2022-12-20T07:54:46Z

+ """
+
+
+def wrapCrudUndefinedProcedureError(e: DatabaseError):


There is no wrapCrudUndefinedProcedureError now, but there is also no exceptions chaining yet and you still overwrite exception fields.

DifferentialOrange · 2022-12-20T07:55:47Z

+ info = ". Ensure that you're calling crud.router and user has sufficient grants"
+ e.message = e.message + info
+ e.extra_info.message = e.extra_info.message + info
+ e.args = (e.args[0], e.args[1] + info)


Either you forgot to commit it or lost it while doing other reworks, but it's still in call_crud

DifferentialOrange · 2022-12-20T07:59:05Z

+
+ def crud_insert(self, space_name, values, opts={}, *, mode=Mode.ANY):
+ """
+ Execute an CRUD_INSERT request on the pool server: 


SUCH_STYLE in other documentation strings refer to IPROTO_REQUEST codes (omitting the IPROTO_ part), so writing CRUD_INSERT here and below may be a bit confusing.
You may ignore this comment if you wish.

Done, rewritten without uppercase

GRISHNOV · 2022-12-21T14:27:58Z

Thank you for your feedback! I have updated the PR description and tried to answer all the comments on the code review

DifferentialOrange

Thank you for your updates! Let's resolve a couple of minor comments and I'll merge this one.

Adds native api support for crud module [1] to use it from a connection object. 1. github.com/tarantool/crud Closes#205

GRISHNOV requested a review from DifferentialOrange November 16, 2022 08:55

DifferentialOrange requested changes Nov 16, 2022
View reviewed changes

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch from ad145c4 to 6343199Compare November 22, 2022 18:18

GRISHNOV requested a review from DifferentialOrange November 22, 2022 20:32

DifferentialOrange reviewed Nov 23, 2022
View reviewed changes

Comment threadtarantool/connection.py
Comment threadtarantool/connection.py
Comment threadtarantool/connection.py Outdated
Comment threadtarantool/error.py Outdated

DifferentialOrange mentioned this pull request Nov 23, 2022
Type hints and type check #266
Open

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch 4 times, most recently from 3ed26c5 to cc04dc4Compare November 29, 2022 12:40

GRISHNOV marked this pull request as ready for review November 29, 2022 13:34

GRISHNOV requested a review from DifferentialOrange November 29, 2022 13:35

DifferentialOrange reviewed Nov 29, 2022
View reviewed changes

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch 13 times, most recently from 20f6c01 to da98380Compare December 5, 2022 21:49

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch from 3057dec to 781c5fbCompare December 9, 2022 09:31

DifferentialOrange reviewed Dec 9, 2022
View reviewed changes

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch 7 times, most recently from b5de7af to 1611fc9Compare December 12, 2022 09:21

GRISHNOV requested a review from DifferentialOrange December 12, 2022 09:27

DifferentialOrange reviewed Dec 13, 2022
View reviewed changes

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch from 1611fc9 to 3455edcCompare December 19, 2022 14:16

GRISHNOV requested a review from DifferentialOrange December 19, 2022 14:23

DifferentialOrange reviewed Dec 20, 2022
View reviewed changes

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch from 3455edc to 2e88041Compare December 21, 2022 13:56

GRISHNOV requested a review from DifferentialOrange December 21, 2022 14:28

DifferentialOrange approved these changes Dec 22, 2022
View reviewed changes

Comment threadtarantool/crud.py Outdated
Comment threadtarantool/crud.py Outdated
Comment threadtarantool/error.py Outdated
Comment threaddocs/source/quick-start.rst

api: native crud module support
832ab10
Adds native api support for crud module [1] to use it from a connection object. 1. github.com/tarantool/crud Closes#205

GRISHNOV force-pushed the igrishnov/gh-205-native-crud-api-support branch from 2e88041 to 832ab10Compare December 22, 2022 08:17

DifferentialOrange approved these changes Dec 22, 2022
View reviewed changes

DifferentialOrange merged commit 4a791dc into masterDec 22, 2022

DifferentialOrange deleted the igrishnov/gh-205-native-crud-api-support branch December 22, 2022 08:45

DifferentialOrange mentioned this pull request Dec 22, 2022
Fix packing in Github Actions #272
Merged

DifferentialOrange mentioned this pull request Feb 13, 2023
api: add CRUD module support tarantool/go-tarantool#263
Merged
24 tasks

Uh oh!

Conversation

GRISHNOV commented Nov 15, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Inserting data via crud:

Crud not found on the router:

Select and unflatten_rows via crud:

Truncate and len via crud:

Encoding=None in Connection:

Uh oh!

DifferentialOrange left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GRISHNOV commented Nov 22, 2022

Uh oh!

DifferentialOrange commented Nov 23, 2022

Uh oh!

GRISHNOV commented Nov 23, 2022

Uh oh!

DifferentialOrange left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GRISHNOV commented Nov 29, 2022

Uh oh!

DifferentialOrange left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DifferentialOrange left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

What's in crud

For non-many

For many

What's in python

How we can mix it

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GRISHNOV commented Nov 15, 2022•
edited
Loading

DifferentialOrange left a comment •
edited
Loading