Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transaction improvement #1054

Open
Qiwn opened this issue Nov 30, 2020 · 4 comments
Open

Transaction improvement #1054

Qiwn opened this issue Nov 30, 2020 · 4 comments

Comments

@Qiwn
Copy link
Contributor

Qiwn commented Nov 30, 2020

Transaction only considers the objects in pg, it doesn't consider those objects are not stored in pg yet (they are in txn.deleted/added dict). It would be great if this kind of cases are contemplated:

async def test_txn_add_and_delete(container_requester):
    async with container_requester as requester:
        async with transaction(db=requester.db) as txn:
            root = await txn.get(ROOT_ID)
            container = await root.async_get("guillotina")
            await create_content_in_container(
                container, "Item", "foobar", check_security=False, __uuid__="foobar"
            )
            await container.async_del("foobar")
            obj = await container.async_get("foobar")  # should be deleted at this point
            assert obj is None

        async with transaction(db=requester.db) as txn:
            obj = await container.async_get("foobar")
            # object still exists
            assert obj is None

Another case: #1053

@masipcat
Copy link
Contributor

masipcat commented Dec 1, 2020

@bloodbare @vangheem @jordic What do you think about this? IMO this is something interesting to implement in G7

@jordic
Copy link
Contributor

jordic commented Dec 2, 2020

From my point of view, this is cleary a bug, if we support "virtual transactions" we should do the correct way, and data within boundaries should be consistent!

@bloodbare
Copy link
Member

Looks like a good idea to clean up the g transaction flow.

In your use case https://github.com/plone/guillotina/blob/master/guillotina/content.py#L358 is the responsible that at async_get gets None because its not on db, so fixing that async_get can retrieve from txn objects (we will need to store the parent and id on the txn virtual state) may be enought to fix this issue.

@bloodbare
Copy link
Member

After the PR the main scope will be fixed. There is still an inconsistency on async_len, aync_keys, async_values .... all this functions will not be consistent with modified objects and may be the scope for a future cache refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants