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

Add UUID type conversion and fix datetime comparison #405

Merged

Conversation

theelderbeever
Copy link
Contributor

Reference Issue #404

Motivation

The querystring builder is valuable when trying to write custom complex queries with VALUES however, certain values are not converted to sql compatible values properly. This results in Table.raw(query).run() failing. Changes here aspire to address this issue.

Explanation

A couple of type check fixes. The datetime comparison in the querystring was comparing to the module not the class and wouldn't be picked up. Additionally, the .replace("T", " ") doesn't work with postgres as far as I can tell and I removed it.

There also needs to be a check on UUID. When being written to a query string the value should be wrapped in ''.

I imagine something similar may need to be done with JSONB, Enum, etc. Maybe there could be some merging of usage with the piccolo.utils.sql_values.convert_to_sql_value function.

Comment on lines 12 to 15
if find_spec("asyncpg"):
from asyncpg.pgproto.pgproto import UUID
else:
from uuid import UUID
Copy link
Member

@dantownsend dantownsend Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally it would check both of these, because for Postgres, a value could be a uuid.UUID or asyncpg.pgproto.pgproto.UUID.

We pass uuid.UUID into queries like INSERT, but asyncpg returns asyncpg.pgproto.pgproto.UUID from SELECT. We're most likely to be dealing with uuid.UUID.

Copy link
Contributor Author

@theelderbeever theelderbeever Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asyncpg only gets installed if the piccolo[postgres] is installed, correct? I am not sure I have a good idea on how the case where it hasn't been installed other than moving the find_spec("asyncpg") into the conversion if-elif-else scope (l# 103). Is that what you would recommend?

Copy link
Contributor Author

@theelderbeever theelderbeever Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or something like

if find_spec("asyncpg"):
    from asyncpg.pgproto.pgproto import UUID as apgUUID
else:
    from uuid import UUID
    apgUUID = UUID

Then do the appropriate typecheck with an or on line 102

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that works.

@dantownsend
Copy link
Member

@theelderbeever Thanks for this. The changes are definitely useful.

The string representation of a query is more of a convenience, so you can do something like print(Band.select()) and immediately see the SQL that would be generated. That's why there might be bugs with it, because Piccolo doesn't use it directly in queries.

I think this issue has also made it clear that an API is needed for composing custom queries with Querystring, like await Manager.run_querystring(querystring). I'll create a separate issue for that.

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

Codecov Report

Merging #405 (6b298e5) into master (1e384dd) will decrease coverage by 0.00%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
- Coverage   90.84%   90.83%   -0.01%     
==========================================
  Files         103      103              
  Lines        6826     6833       +7     
==========================================
+ Hits         6201     6207       +6     
- Misses        625      626       +1     
Impacted Files Coverage Δ
piccolo/querystring.py 89.87% <77.77%> (-0.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e384dd...6b298e5. Read the comment docs.

@dantownsend
Copy link
Member

@theelderbeever Thanks, looks good 👍

@dantownsend dantownsend merged commit 439afcd into piccolo-orm:master Jan 25, 2022
@dantownsend dantownsend added the enhancement New feature or request label Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants