Skip to content

Fix JSON type + update Enum #72

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 10, 2019
Merged

Fix JSON type + update Enum #72

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7bd2ace
Merge pull request #2 from dropbox/master
mehdigmira Feb 3, 2019
6317b79
Merge pull request #3 from dropbox/master
mehdigmira Feb 9, 2019
0ebca58
Fix JSON type: json can either be a Dict or a List
Feb 9, 2019
ad6551b
Add enums attribute to Enum
Feb 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions sqlalchemy-stubs/sql/sqltypes.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ class Enum(Emulated, String, SchemaType):
native_enum: bool = ...
create_constraint: bool = ...
validate_strings: bool = ...
enums: List[typing_Text] = ... # This is set inside _setup_for_values.
def __init__(self, *enums: Any, **kw: Any) -> None: ...
@property
def native(self) -> bool: ...
Expand Down Expand Up @@ -243,7 +244,9 @@ class Interval(Emulated, _AbstractInterval[timedelta], TypeDecorator[timedelta])
def bind_processor(self, dialect: Dialect) -> Callable[[Optional[timedelta]], str]: ...
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]]
Copy link
Contributor

@ilevkivskyi ilevkivskyi Feb 16, 2019

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@mehdigmira mehdigmira Feb 16, 2019

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

@ilevkivskyi ilevkivskyi Mar 8, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mehdigmira mehdigmira Mar 10, 2019

Choose a reason for hiding this comment

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

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 ?


class JSON(Indexable, TypeEngine[_JSONT]):
__visit_name__: str = ...
hashable: bool = ...
NULL: util.symbol = ...
Expand All @@ -257,12 +260,13 @@ class JSON(Indexable, TypeEngine[Dict[str, Any]]):
class JSONIndexType(JSONElementType): ...
class JSONPathType(JSONElementType): ...
comparator_factory: Type[Any] = ...
# This method returns `dict`, but it seems to be a bug in SQLAlchemy.
@property
def python_type(self) -> Type[Dict[str, Any]]: ...
@property
def should_evaluate_none(self) -> bool: ... # type: ignore # incompatible with supertype "TypeEngine"
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]]: ...
Copy link
Contributor

@ilevkivskyi ilevkivskyi Feb 16, 2019

Choose a reason for hiding this comment

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

Returning more precise types here are probably fine however.


class ARRAY(Indexable, Concatenable, TypeEngine[List[_T]]):
__visit_name__: str = ...
Expand Down