sqlalchemy / sqlalchemy

The Database Toolkit for Python
https://www.sqlalchemy.org
MIT License
9.05k stars 1.36k forks source link

Use of `raise NotImplementedError` instead of `NotImplemented` #11349

Closed zzzeek closed 1 month ago

zzzeek commented 1 month ago

Discussed in https://github.com/sqlalchemy/sqlalchemy/discussions/10645

Originally posted by **fazledyn-or** November 16, 2023 I am here to discuss about an issue that we came across while triaging your project. ## Background While traiging your project, our bug fixing tool generated the following message(s)- In file: [associationproxy.py](https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/ext/associationproxy.py#L1980), class: _AssociationSet, there is a special method `__ixor__` that raises a NotImplementedError. If a special method supporting a binary operation is not implemented it should return [NotImplemented](https://docs.python.org/3/library/constants.html#NotImplemented). On the other hand, [NotImplementedError](https://docs.python.org/3/library/exceptions.html#NotImplementedError) should be raised from abstract methods inside user defined base classes to indicate that derived classes should override those methods. iCR suggested that the special method `__ixor__` should return NotImplemented instead of raising an exception. An example of how NotImplemented helps the interpreter support a binary operation is [here](https://docs.python.org/3/library/numbers.html#implementing-the-arithmetic-operations). The same message is generated for `__imul__`, `__iand__`, `__isub__`, `__ior__` methods too. ## Details The reason behind suggesting this fix is that developers often confuse `NotImplemented` and `NotImplementedError` equivalent, which is wrong. Using `NotImplemented` allows the Python interpreter to take over and try performing that operation. If it fails to do so, the interpreter throws a `TypeError`. ## Suggested Changes It is suggested that `raise NotImplementedError` be replaced with `return NotImplemented`. If you're willing, I can create a fix and submit a PR. ## Previously Found & Fixed - https://www.github.com/SciTools/iris/pull/5544 - https://www.github.com/cupy/cupy/pull/7900 - https://www.github.com/ethereum/web3.py/pull/3080 ## Sponsorship and Support This work is done by the security researchers from OpenRefactory and is supported by the [Open Source Security Foundation (OpenSSF)](https://openssf.org/): [Project Alpha-Omega](https://alpha-omega.dev/). Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security. The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.
sqla-tester commented 1 month ago

Mike Bayer has proposed a fix for this issue in the main branch:

correctly apply _set_binops_check_strict to AssociationProxy https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5279