empty result provides a len of 0

[drfho]

Hello @dataflake, ZSQLMethods provides on UPDATE.SQL-calls (with SQLAlchemyDA) an empty resultset (NoneType) and then Results.py runs into an type error: object of type 'NoneType' has no len() blocking an succesful execution of SQL-UPDATE. My code proposal makes sure, that the len() provides a valid value (0) in case the resultset is empty.

Comment: SQLAlchemyDA may not generate the expected result object, but I think it is more stable to cover this case in Products.ZSQLMethods.

Hope you like it. f

Pic-1: SQL-UPDATE returns a NoneType-result having no length

Pic-2: Effect of the code-fix (right)

[dataflake]

I see several issues with this patch:

First of all, I don't think this is a bug in this package and removed the bug label here. My guess that SQLAlchemyDA is doing it wrong.

Secondly, before requesting a review you must ensure that the tests run through. If you don't want to wait for the automated tests here you can always install tox into your virtual environment and then run it.

As far as the specific implementation is concerned, I would not do it that way. Enclose the len call in a try/except and look for TypeError (and I suggest AttributeError as well to cover other odd cases) and in the except block return 0.

[drfho]

please, excuse me for labelling it as a bug (for me on first sight the other labels didn't fit either; next time I will choose "draft" in similar cases ;-). Of course the situation is due to SQLAlchemyDA's type handling. On the other hand, I am not sure whether we should force Products.ZSQLMethods to throw a lot of useless errors if a third party module just returns a None. Maybe other people consider it as legitimate returning None if there is actually not an empty result list but really no result. Tests are successful now, sorry tor that. Cheers f

[dwt]

Just checking in from the side: a __len__() method that returns anything else besides integer sounds like a total footgun to me, it's just not how __len__() ought to behave.

Not sure what the right solution to this problem is, as I don't really understand this code, but I would really like to be able to have an empty result have the length 0 to prevent code paths that are just for error checking.

[dataflake]

@dwt I don't understand your comment. The original code calls __len__ on self._data, which is initialized to something invalid - None - by ZSQLAlchemyDA. It's perfectly valid to react to this foul by throwing an exception. So the old code will either return a valid value or throw an exception, which sounds fine to me. It never returns anything invalid.

The real solution would be to fix ZSQLAlchemyDA to correctly initialize the value that gets put into self._data. The second best is putting error correction into ZSQLMethods, which Frank is trying to do, just not in a good way.

[dataflake]

Of course the situation is due to SQLAlchemyDA's type handling. On the other hand, I am not sure whether we should force Products.ZSQLMethods to throw a lot of useless errors if a third party module just returns a None.

All this code (including ZSQLAlchemyDA I believe) is literally decades old and this issue has not surfaced before. Could this be something specific to your setup? Have you modified ZSQLAlchemyDA or added any code that messes with result sets that come back from the database?

[dwt]

@dataflake Do you think that fixing this problem here would expose other users of this package that expect len and other methods to throw if _data is None?

The problem, as far as I understand it, is that an update statement does not really have a result (except maybe the number of affected lines). Thus if the query is triggered in Shared.DC.ZRDB.DA then SQLAlchemy will return the None for the affected rows (as it knows none) in Products.SQLAlchemyDA.da: SAWrapper.query

Now where is the right place to fix this? I think it canot be a consumer of the Result (i.e checking that _len() doesn't raise) as in this example the template processor triggers and catches that. Fixing it here is relatively simple as it just says, no result has a len of 0 - which makes some sense. One could also fix it in [Shared.DC.ZRDB.DA:DA.call__()](https://github.com/zopefoundation/Products.ZSQLMethods/blob/3f8771afe37f70a1f694e5b7e1eb08ec03b54aaa/src/Shared/DC/ZRDB/DA.py#L739) as in creating the Results() class differently if the database does not return a result (common for update queries). Or at lasts one could fix in .Products.SQLAlchemyDA.da:SAWrapper.query

I am not sure where the best place to fix this would be, as the contract between these classes - especially for update statements - is not obvious to me. As you seem to have a lot of commits here, how do you interpret the contract between these classes? Which of them is behaving wrong?

I would guess that making the Results() object able to deal with None _data (and that would require more changes than the one provided in this pull request so far) is a relatively easy way to deal with all of this that will likely make the class more robust - but that is just my guess.

How do you read this?

[drfho]

Here I propose an alternative fix making sure that SQLAlchemyDA provides the expected type of the data object: https://github.com/zopefoundation/Products.SQLAlchemyDA/pull/12

[d-maurer]

?rekcäH nitraM? wrote at 2021-11-1 10:36 -0700:

... I am not sure where the best place to fix this would be, as the contract between these classes - especially for update statements - is not obvious to me.

For update statements, the user of the ZSQLMethod should not call len on the result. If he does, it is acceptable that he gets an exception.

[drfho]

For update statements, the user of the ZSQLMethod should not call len on the result. If he does, it is acceptable that he gets an exception.

Many thanks for your input: The problem here is that applying the len()-function somewhere in your own code is not needed to get the error. This is implicitly done by ZSQLMethod itself when called an sql-updating-ZSQLMethod by a PyScript or a DTMLMethod. So with Products.SQLAlchemyDA you cannot execute any sql-updates by other Zope objects like PyScripts (it just ends up into the type error, no sql-update).

[dataflake]

@drfho Can you post a full traceback?

[drfho]

Thanks for asking, here the documentation of the error case and the traceback.log:

Pic1: two Zope objects // Py-Script calls ZSQLmethod

Pic2: ZMySQLDA self._data = ()

Pic3: SQLAlchemyDA self._data = None

Traceback of Viewing the PyScript calling the ZSQLMethod (using SQLAlchemyDA) traceback.log

[dwt]

I guess it should be possible by actually trying to display the result of this test call via a template. Maybe something like that is what is missing to expose this problem?

[dataflake]

After reviewing the code in this package, which clearly treats self._data as an iterable in several places, and checking what Products.ZMySQLDA does (it returns two tuples) the fix should go into Products.SQLAlchemyDA. I proposed one small change at https://github.com/zopefoundation/Products.SQLAlchemyDA/pull/12

[drfho]

Great, so I close this PR Many thanks to everyone! f