[Sqlalchemy-tickets] Issue #4353: raiseload with sql_only=False is overly aggressive (zzzeek/sqlalchemy)

[Alex R. <iss...@bi...>]

New issue 4353: raiseload with sql_only=False is overly aggressive
https://bitbucket.org/zzzeek/sqlalchemy/issues/4353/raiseload-with-sql_only-false-is-overly

Alex Rothberg:

I believe that I am seeing a case where `raiseload` with `sql_only=False` is triggering an exception when it should not be. While setting `sql_only=True` avoids the error for reasons outlined below this is not an ideal workaround. Further, exception is being generated in a case where the look up is entirely not needed (explanation below). The stack trace is:

```
#!python

/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/attributes.py:229: in __set__
    instance_dict(instance), value, None)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/attributes.py:1079: in set
    initiator=evt)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/collections.py:768: in bulk_replace
    existing_adapter.fire_remove_event(member, initiator=initiator)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/collections.py:698: in fire_remove_event
    item, initiator)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/attributes.py:962: in fire_remove_event
    fn(state, value, initiator or self._remove_token)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/attributes.py:1255: in emit_backref_from_collection_remove_event
    passive=PASSIVE_NO_FETCH)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/attributes.py:634: in pop
    passive=passive, check_old=value, pop=True)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/attributes.py:800: in set
    LOAD_AGAINST_COMMITTED)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/attributes.py:599: in get
    value = callable_(state, passive)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/strategies.py:834: in __call__
    return strategy._load_for_state(state, passive)
/Users/alex/.pyenv/versions/app/lib/python3.7/site-packages/sqlalchemy/orm/strategies.py:589: in _load_for_state
    self._invoke_raise_load(state, passive, "raise")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <sqlalchemy.orm.strategies.LazyLoader object at 0x1176b2f20>
state = <sqlalchemy.orm.state.InstanceState object at 0x1162462b0>, passive = 53
lazy = 'raise'
```

I believe that the problem is that an exception is thrown unconditionally here: https://github.com/zzzeek/sqlalchemy/blob/7d372da7385be6a9817a20b6b62f7c4237af7b26/lib/sqlalchemy/orm/strategies.py#L588-L589 even if this escape hatch would have been hit: https://github.com/zzzeek/sqlalchemy/blob/7d372da7385be6a9817a20b6b62f7c4237af7b26/lib/sqlalchemy/orm/strategies.py#L626-L628

I think in the cases where the load is entirely unneeded (as it is in my case), the exception should not be raised.

While a workaround is to set `sql_only=True`, I want to avoid that as it can lead to differences is results between testing and production (due to the presence of items in the identity map avoiding exceptions).

The reason I say the load us unneeded is that sqla is attempting to load the old value in a soon to be replaced backref (ie this is part of the emit).