Fix JSON type + update Enum #72
Conversation
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks for PR! I have couple comments.
| def result_processor(self, dialect: Dialect, coltype: Any) -> Callable[[Optional[str]], Optional[timedelta]]: ... | ||
|
|
||
| class JSON(Indexable, TypeEngine[Dict[str, Any]]): | ||
| _JSONT = Union[Dict[str, Any], List[Any]] |
There was a problem hiding this comment.
Did this cause some problems in actual code? If this is made from the point of view of general correctness, then I wouldn't do this.
Even if it caused some problems, I think this will likely cause lots of false positives, reading this attribute from a model will need to be always followed by an isinstance(..., dict).
There was a problem hiding this comment.
We have multiple JSON fields that look like [{"from": 1, "to": "3", "name": "x"}, ...]. So in our case it created a lot of false negatives.
In PostgreSQL (and hence SQLAlchemy) there as many operators to handle [{"from": 1, "to": "3", "name": "x"}, ...] as there are for {"from": 1, "to": "3", "name": "x"}. So I thinks It's a very common practice to have lists. https://www.postgresql.org/docs/9.3/functions-json.html
Maybe a solution would be to give a hint when creating the column ?
There was a problem hiding this comment.
Sorry for a delay, coming back to this.
So in our case it created a lot of false negatives.
I would say that false negatives are less important that false positives, the other changes in this PR are OK however.
There was a problem hiding this comment.
Sorry, I meant false positives. Again, a JSON field is not a dict. It is very common to store smth like [{"from": 1, "to": "3", "name": "x"}, ...]. It is both supported by Postgres and SQLAlchemy.
This causes any code such as MyModel.my_json_col[0] to be marked as an error. Adding a type ignore to every occurence is not an option IMO.
What is the rational behind setting JSONB to be a dict ?
| def bind_processor(self, dialect: Dialect) -> Callable[[Optional[Dict[str, Any]]], Optional[str]]: ... | ||
| def result_processor(self, dialect: Dialect, coltype: Any) -> Callable[[Optional[str]], Optional[Dict[str, Any]]]: ... | ||
| def bind_processor(self, dialect: Dialect) -> Callable[[Optional[_JSONT]], Optional[str]]: ... | ||
| def result_processor(self, dialect: Dialect, coltype: Any) -> Callable[[Optional[str]], Optional[_JSONT]]: ... |
There was a problem hiding this comment.
Returning more precise types here are probably fine however.
|
I finally had time time to check this against our internal code bases and it looks like this is clean, so can be merged now. Sorry for a delay. |
|
I just want to comment in case anyone else is upgrading and is seeing this as a pretty big breaking change for them, resulting in many errors that look like this: "error: No overload variant of "getitem" of "list" matches argument type "str" [call-overload]". An easy fix is to do the following for |
3 participants
enumsattribute toEnumclass