strawberry-graphql / strawberry-sqlalchemy

A SQLAlchemy Integration for strawberry-graphql
MIT License
91 stars 26 forks source link

Mapping BigInteger to int limits the max value of BigInteger #100

Closed IdoKendo closed 9 months ago

IdoKendo commented 9 months ago

Describe the Bug

I have a sqlalchemy model that is set up to something like this:

class UsersModel(Base):
    __tablename__ = "users"

    id = Column(BigInteger, unique=True, primary_key=True)
    # ... other fields

and I am mapping the schema like this:


@mapper.type(UsersModel)
class Users:
    ...

Some of the IDs that are being used are very large, e.g.: 1_029_320_583_927 which is way beyond the limit of Int32 (2_147_483_647), thus when I run a pretty simple query:

query {
  users {
    id
    name
  }
}

I am getting an error: Int cannot represent non 32-bit signed integer value: 1029320583927

I think the mapping should be done to a custom scalar type in order to allow big integer values.

System Information

Additional Context

Unfortunately the IDs must be BigInteger since this is already being used by other services so I can't change it to be UUID or even String which might be a different approach to solve it.

Upvote & Fund

Fund with Polar

erikwrede commented 9 months ago

Good catch, in graphene-sqlalchemy we are using floats for this. The disadvantage of using floats is seemingly allowing non-decimal values which fail later during insert, but it's a common approach to handle long ints. The other approach is to add a Long scalar. Do you have any preferences on this? /cc @mattalbr @bellini666

IdoKendo commented 9 months ago

For my use case, both solutions would work, so no preferences on my end.

IdoKendo commented 9 months ago

@erikwrede I see that in the strawberry documentation: https://strawberry.rocks/docs/types/scalars#int64 it is addressed in this fashion:

Int64 = strawberry.scalar(
    Union[int, str],  # type: ignore
    serialize=lambda v: int(v),
    parse_value=lambda v: str(v),
    description="Int64 field",
)

I suppose this would be an adequate solution, WDYT?

erikwrede commented 9 months ago

I think this issue is a great motivator to enable customization of type conversion rules for this library, but for a quick fix Int64 is fine 😊

@IdoKendo do you want to submit a PR including 1 or 2 test cases for this change? 🙂

IdoKendo commented 9 months ago

Yeah I'll try to work on a PR 💪 Thanks!