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

Fix some column types being parsed twice #582

Merged
merged 4 commits into from
Feb 22, 2024

Conversation

pmdevita
Copy link
Contributor

After the SQLAlchemy 2.0 fixes, ormar's test suite highlighted a problem with JSON and enum types getting parsed from the database a second time. This should fix the problem. I'm not sure if there are any other data types affected at the moment, it should hopefully be just these two. I've also added appropriate tests for these data types.

Let me know if I need to change anything, thanks!

@tarsil
Copy link
Contributor

tarsil commented Feb 21, 2024

After the SQLAlchemy 2.0 fixes, ormar's test suite highlighted a problem with JSON and enum types getting parsed from the database a second time. This should fix the problem. I'm not sure if there are any other data types affected at the moment, it should hopefully be just these two. I've also added appropriate tests for these data types.

Let me know if I need to change anything, thanks!

This looks ok to me. Let us get a second opinion on this while I also check it.

Thank you for the submission.

@pmdevita
Copy link
Contributor Author

pmdevita commented Feb 21, 2024

TIME also needed to be fixed, added a test for it along with DATE

EDIT: OK custom types on the Postgres backend were also being affected. I've changed the logic to be inclusive rather than exclusive. Only primitive types with a Postgres backend are processed now.

@pmdevita pmdevita changed the title Fix JSON and enum type columns Fix some column types being parsed twice Feb 21, 2024
@tarsil
Copy link
Contributor

tarsil commented Feb 22, 2024

@Kludex I did some research and SQLA did some internal changes after the version 2.0.24 and I'm actually happy with these changes as well. If you agree, we can move forward.

@Kludex
Copy link
Member

Kludex commented Feb 22, 2024

Feel the owner here. I trust you.

@tarsil
Copy link
Contributor

tarsil commented Feb 22, 2024

@pmdevita thank you for your submission here as it was a nice catch from ormar. So far the changes you have applied, it seems like its covering the scenarios properly.

@tarsil tarsil merged commit 0fc16b2 into encode:master Feb 22, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

3 participants