MolSSI / QCFractal

A distributed compute and database platform for quantum chemistry.
https://molssi.github.io/QCFractal/
BSD 3-Clause "New" or "Revised" License
144 stars 47 forks source link

Generalized upsert functionality in socket #652

Closed bennybp closed 3 years ago

bennybp commented 3 years ago

Description

There are many functions in the socket that manually perform upserts (update or insert). This pulls that functionality into a general function, hopefully cleaning up a lot of existing code and making it less error prone.

This will possibly enable a future optimization, where we use postgres upsert functionality (ON CONFLICT DO ...), increasing the performance. For now, this function is doing the slower, but more explicit way.

This PR adds two functions: One for upsert, and one for a pure insert that checks for existing records based on search keys. These new functions are used for kv_store (upsert), and the keywords table (insert).

This PR also adds an improved way for returning metadata from the socket. This is implemented via dataclasses rather than dictionaries, and via multiple return. This is done for the kvstore functions. The hope is to move to a more standardized way of reporting this information outside the socket.

Because the add_keywords function is more user-facing, the return is left as is, which a shim converting to the old type of metadata. This has the benefit of not requiring changes anywhere else in the code including tests (for now).

Changelog description

Start using common upsert/insert functions in the storage socket

Status

codecov[bot] commented 3 years ago

Codecov Report

Merging #652 (0c77173) into master (78e4b18) will increase coverage by 11.12%. The diff coverage is 90.04%.

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 6d151c1e61303a2a23dafd1c0a721ee23b246a45 into a8e0af89c4500dfa965ff93ee8dfc51850b15502 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 55c15bf516b48959c802b18e46fdbf3d4bf45d77 into a8e0af89c4500dfa965ff93ee8dfc51850b15502 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging b0ec81f1cd4ee7d405ed2008c4883ff3fa565b56 into a8e0af89c4500dfa965ff93ee8dfc51850b15502 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 2 alerts when merging 03ee08dc6bf73d0bc32f22b925e0d34c2dc04d0e into a8e0af89c4500dfa965ff93ee8dfc51850b15502 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 819edac827cec028f0caa18064279255c38bd380 into a8e0af89c4500dfa965ff93ee8dfc51850b15502 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 2 alerts when merging 42c794552677551fe59577317a91c6d55eec7f75 into a8e0af89c4500dfa965ff93ee8dfc51850b15502 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 462bb05862cfbe59665a2c44d253fe00fdd3078c into a8e0af89c4500dfa965ff93ee8dfc51850b15502 - view on LGTM.com

new alerts:

bennybp commented 3 years ago

Ready for review. I prefer to keep this small-ish to make review easier :)

The next PR will use this functionality to fix a few small bugs in services related to duplicating kvstore rows.

bennybp commented 3 years ago

Closing since this is outdated. Going a slightly different direction in the next branch