Added typecheck to sqlalchemy JSON #931
Conversation
In sqlmodel/main.py there was missing a typecheck for the JSON type supported by sqlalchemy. So it was not possible to define a model with a JSON field.
|
please add JSONB support too |
|
Ah, never mind. Found a fix: from sqlmodel import SQLModel, Field, JSON
class User(SQLModel, table=True):
id: int = Field(..., primary_key=True)
data: JSON = Field(default={}, sa_type=JSON) |
|
Hi @vduseev @LucaGelmini is there an update on when this can be merged? |
|
As I see this can be merged right now. The JSON compatibility was supported by SQLAlchemy, just that it wasn't implemented inside the SQLModel map of the SQLAlchemy types. So, I just changed two lines to include the JSON class 😂. Very straightforward change. |
|
First collaboration in a repo though, so if I'm missing something please tell me 🫡 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
returning the sqlalchemy Uuid type
|
Really want this to be merged 🙏 |
| if issubclass(type_, uuid.UUID): | ||
| return Uuid | ||
| if issubclass(type_, JSON): | ||
| return JSON |
There was a problem hiding this comment.
Don't you want issubclass(type_, Mapping) rather here?
Also it is probably not clear if JSON or JSONB is the desired type.
Maybe better to document the workaround in the docs
There was a problem hiding this comment.
Shouldn't it be if issubclass(type_, pydantic.Json): instead?
Or better
if issubclass(type_, (Sequence, Mapping, Json)):
return JSON
YuriiMotov
left a comment
YuriiMotov
left a comment
There was a problem hiding this comment.
@LucaGelmini, thanks for your interest!
Are you still ready to work on this?
We need to cover this by tests at least.
But I would actually extend this to also support Sequence and Mapping
| if issubclass(type_, uuid.UUID): | ||
| return Uuid | ||
| if issubclass(type_, JSON): | ||
| return JSON |
There was a problem hiding this comment.
Shouldn't it be if issubclass(type_, pydantic.Json): instead?
Or better
if issubclass(type_, (Sequence, Mapping, Json)):
return JSON
github-actions
bot
added
the
conflicts
|
This pull request has a merge conflict that needs to be resolved. |
|
As this PR has been waiting for the original user for a while but seems to be inactive, it's now going to be closed. But if there's anyone interested, feel free to create a new PR. |
|
Is anybody up to take this over? |
8 participants
In sqlmodel/main.py there was missing a typecheck for the JSON type supported by sqlalchemy. So it was not possible to define a model with a JSON field.