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

.upsert_all() should maybe error if dictionaries passed to it do not have the same keys #99

Closed
simonw opened this issue Apr 13, 2020 · 2 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@simonw
Copy link
Owner

simonw commented Apr 13, 2020

While investigating #98 I stumbled across this:

    def test_upsert_compound_primary_key(fresh_db):
        table = fresh_db["table"]
        table.upsert_all(
            [
                {"species": "dog", "id": 1, "name": "Cleo", "age": 4},
                {"species": "cat", "id": 1, "name": "Catbag"},
            ],
            pk=("species", "id"),
        )
        table.upsert_all(
            [
                {"species": "dog", "id": 1, "age": 5},
                {"species": "dog", "id": 2, "name": "New Dog", "age": 1},
            ],
            pk=("species", "id"),
        )
>       assert [
            {"species": "dog", "id": 1, "name": "Cleo", "age": 5},
            {"species": "cat", "id": 1, "name": "Catbag", "age": None},
            {"species": "dog", "id": 2, "name": "New Dog", "age": 1},
        ] == list(table.rows)
E       AssertionError: assert [{'age': 5, '...cies': 'dog'}] == [{'age': 5, '...cies': 'dog'}]
E         At index 0 diff: {'species': 'dog', 'id': 1, 'name': 'Cleo', 'age': 5} != {'species': 'dog', 'id': 1, 'name': None, 'age': 5}
E         Full diff:
E         - [{'age': 5, 'id': 1, 'name': 'Cleo', 'species': 'dog'},
E         ?                              ^^^ --
E         + [{'age': 5, 'id': 1, 'name': None, 'species': 'dog'},
E         ?                              ^^^
E         {'age': None, 'id': 1, 'name': 'Catbag', 'species': 'cat'},
E         {'age': 1, 'id': 2, 'name': 'New Dog', 'species': 'dog'}]

If you run .upsert_all() with multiple dictionaries it doesn't quite have the effect you might expect.

@simonw simonw added the bug Something isn't working label Apr 13, 2020
@simonw
Copy link
Owner Author

simonw commented Apr 13, 2020

I think I'm going to leave this as intended behaviour. Or maybe passing multiple dictionaries to .upsert_all() with different numbers of keys should raise an error?

@simonw simonw changed the title .upsert_all() nulls out fields .upsert_all() should error if dictionaries passed to it do not have the same keys Apr 13, 2020
@simonw simonw added the wontfix This will not be worked on label Apr 13, 2020
@simonw
Copy link
Owner Author

simonw commented Apr 13, 2020

Bit trick from an implementation point of view this, since we want to be able to handle input that is a generator - so we can't scan through the input to validate that every dictionary has the same exact keys without consuming the entire iterator.

The alternative would be to raise an error the first time we spot a dictionary with keys that differ... but that's weird because we commit changes in batches, so we may end up only applying half of the changes before exiting with the error.

On that basis, I'm going to leave this as-is and mark this as wontfix.

@simonw simonw closed this as completed Apr 13, 2020
@simonw simonw changed the title .upsert_all() should error if dictionaries passed to it do not have the same keys .upsert_all() should maybe error if dictionaries passed to it do not have the same keys Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

1 participant