Skip to content

Allow SQLAlchemy class mapping errors to propagate #281

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
65 changes: 65 additions & 0 deletions graphene_sqlalchemy/tests/test_types.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import mock
import pytest
import six # noqa F401
import sqlalchemy.exc
import sqlalchemy.orm.exc

from graphene import (Dynamic, Field, GlobalID, Int, List, Node, NonNull,
ObjectType, Schema, String)
from graphene.relay import Connection

from .. import utils
from ..converter import convert_sqlalchemy_composite
from ..fields import (SQLAlchemyConnectionField,
UnsortedSQLAlchemyConnectionField, createConnectionField,
Expand Down Expand Up @@ -492,3 +495,65 @@ class Meta:
def test_deprecated_createConnectionField():
with pytest.warns(DeprecationWarning):
createConnectionField(None)


@mock.patch(utils.__name__ + '.class_mapper')
def test_unique_errors_propagate(class_mapper_mock):
# Define unique error to detect
class UniqueError(Exception):
pass

# Mock class_mapper effect
class_mapper_mock.side_effect = UniqueError

# Make sure that errors are propagated from class_mapper when instantiating new classes
error = None
try:
class ArticleOne(SQLAlchemyObjectType):
class Meta(object):
model = Article
except UniqueError as e:
error = e

# Check that an error occured, and that it was the unique error we gave
assert error is not None
assert isinstance(error, UniqueError)


@mock.patch(utils.__name__ + '.class_mapper')
def test_argument_errors_propagate(class_mapper_mock):
# Mock class_mapper effect
class_mapper_mock.side_effect = sqlalchemy.exc.ArgumentError

# Make sure that errors are propagated from class_mapper when instantiating new classes
error = None
try:
class ArticleTwo(SQLAlchemyObjectType):
class Meta(object):
model = Article
except sqlalchemy.exc.ArgumentError as e:
error = e

# Check that an error occured, and that it was the unique error we gave
assert error is not None
assert isinstance(error, sqlalchemy.exc.ArgumentError)


@mock.patch(utils.__name__ + '.class_mapper')
def test_unmapped_errors_reformat(class_mapper_mock):
# Mock class_mapper effect
class_mapper_mock.side_effect = sqlalchemy.orm.exc.UnmappedClassError(object)

# Make sure that errors are propagated from class_mapper when instantiating new classes
error = None
try:
class ArticleThree(SQLAlchemyObjectType):
class Meta(object):
model = Article
except ValueError as e:
error = e

# Check that an error occured, and that it was the unique error we gave
assert error is not None
assert isinstance(error, ValueError)
assert "You need to pass a valid SQLAlchemy Model" in str(error)
8 changes: 4 additions & 4 deletions graphene_sqlalchemy/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,10 @@ def __init_subclass_with_meta__(
_meta=None,
**options
):
assert is_mapped_class(model), (
"You need to pass a valid SQLAlchemy Model in " '{}.Meta, received "{}".'
).format(cls.__name__, model)

if not is_mapped_class(model):
Copy link
Contributor

Choose a reason for hiding this comment

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

assert is only evaluated when the interpreter is running in "debug" mode and is ignored when optimizations are enabled with the -O command line switch. I personally think it's proper to have this check every time and what you have written is an improvement, but to match the previous behavior it might be best to wrap this in an if __debug__: check.

https://docs.python.org/3/reference/simple_stmts.html#grammar-token-assert-stmt

Perhaps

if __debug__ and not is_mapped_class(model):
    raise ValueError(...)

raise ValueError(
"You need to pass a valid SQLAlchemy Model in " '{}.Meta, received "{}".'.format(cls.__name__, model)
)
if not registry:
registry = get_global_registry()

Expand Down
7 changes: 6 additions & 1 deletion graphene_sqlalchemy/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ def get_query(model, context):
def is_mapped_class(cls):
try:
class_mapper(cls)
except (ArgumentError, UnmappedClassError):
except ArgumentError as error:
# Only handle ArgumentErrors for non-class objects
if "Class object expected" in str(error):
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, this is a bit of a code-smell. Is it possible to instead create a new exception class and catch that instead? I'm not sure what the import tree looks like, so I'm unsure where it should go, but perhaps:

class InvalidSQLAlchemyModel(ValueError):
    pass

...

    raise InvalidSQLAlchemyModel("You need to pass a valid SQLAlchemy Model in ... ")

Then there's no need for checking the string of the error message here.

Copy link
Author

Choose a reason for hiding this comment

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

I agree but I think that would need to be handled in SQLAlchemy itself

return False
raise error
except UnmappedClassError:
return False
else:
return True
Expand Down