python-gino / gino

GINO Is Not ORM - a Python asyncio ORM on SQLAlchemy core.
https://python-gino.org/
Other
2.68k stars 150 forks source link

ModelLoader issue #717

Closed EldanGS closed 3 years ago

EldanGS commented 4 years ago

Description

  1. Trying to find any field in the response with a null value, f.e example: "name": null
  2. Used two different request 2.1. request to get user(id, name) items by id 2.2. request to get user(name) item by id, expected None value item in the model but, got an exception.
  3. Expected the same result, but got different.

What I Did

Examples of bd & query:

from gino import Gino
import asyncio

db = Gino()

class User(db.Model):
    __tablename__ = "users"
    id = db.Column(db.Integer(), primary_key=True)
    name = db.Column(db.Unicode())

async def main():
    await db.set_bind("postgresql://localhost/gino")
    await db.gino.create_all()
    user_temp = await User.create()

    # 1st query to get user items by id
    user = (
        await User.query.where(User.id == user_temp.id)
            .with_only_columns([User.name, User.id])
            .gino.first()
    )
    assert user.id == user_temp.id

    # 2nd query to get only name of user by id, excepted None value item
    user = (
        await User.query.where(User.id == user_temp.id)
            .with_only_columns([User.name])
            .gino.first()
    )

    assert user is not None  # "fails here due to only empty column(s) are fetched"
    assert user.name is None

if __name__ == "__main__":
    asyncio.run(main())

I guess it happens because of:

class ModelLoader(Loader):
 ... 
 def _do_load(self, row):
 if all((v is None) for v in values.values()):
 return None
wwwjfy commented 4 years ago

Thanks for reporting. There is a gap from http request/response to gino query. Could you help to see what's the values passing to gino engine?

EldanGS commented 4 years ago

Thanks for reporting. There is a gap from http request/response to gino query. Could you help to see what's the values passing to gino engine?

Thanks for the response @wwwjfy, I've updated the description and added the code snippet for a detailed explanation.

wwwjfy commented 4 years ago

@EldanGS Right, it's expected behavior. For detailed explanation, refer to https://github.com/python-gino/gino/blob/master/HISTORY.rst#2-none_as_none

EldanGS commented 4 years ago

@wwwjfy, Is it possible to disable this ability? It could be in many use cases preferable to get an instance with None value rather than just None. Because if someone querying to DB with some attributes (like a name in my example) then expected an instance of DB with ANY value. It would mean that the element is empty (None), not that it does not exist in the DB.

wwwjfy commented 4 years ago

@EldanGS it's not possible in the latest release. This is to prevent the potential crash if we manipulate the None object using loaders. On your side, could you add a check of None and create an empty object? Returning an object with no valid values doesn't make much difference IMHO. I'd rather put this in the app to keep the library simpler.

EldanGS commented 4 years ago

@wwwjfy We can get the right answer through several merger operations, etc. But this will not be an explicit work with the database and ORM.

EldanGS commented 4 years ago

@fantix Hi there, I would like to bring your attention to this issue, maybe you've another point of view. Could you give some suggestions for that?

wwwjfy commented 4 years ago

@EldanGS sorry for the late reply.

I think it does make sense to return empty object in the case that the row exists but the queried columns are empty. Let me try to see if it's possible, without affecting model loader.

EldanGS commented 4 years ago

@wwwjfy No problem, thank you for your reply.

You'll also add TODO that: https://github.com/python-gino/gino/blob/master/HISTORY.rst#2-none_as_none should be changed too in the future or I'll make PR for that.

peace, Eldan

wwwjfy commented 4 years ago

If possible, I don't want to expose the option again but react differently. can check my PR as an attempt to address this.