Bug in MSSQL driver (more than 2000 approvals)

[JohnieBraaf]

Hi,

Sadly I've stubled upon another (critical) bug. This time in the part that fetches the available approvals. https://github.com/javrasya/django-river/blob/master/river/driver/mssql_driver.py#L20-L32

A list of approvals is fetched and then used as a filter on the id array for the TransactionApproval model. When the list of id's is larger than 2000, SQL server runs into a limitation:

• ('42000', '[42000] [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]The incoming request has too many parameters. The server supports a maximum of 2100 parameters. Reduce the number of parameters and resend the request. (8003) (SQLExecDirectW)')

My observation is that this part of the framework is quite inefficient, as I only want the approvals related to a single object and yet it is fetching all approvals and the again uses this to fire an additional parameterized query.

Is there a reasoning behind this?

[JohnieBraaf]

I've written a simple fix for the above described issue, by pushing down the workflow object id into the query. Please see commit: https://github.com/JohnieBraaf/django-river/commit/58f49624c37e1e5d1bdb4c7680810126454e36a6 I see that I have left some debugging code, so please ignore that.

Let me know what you think @javrasya

[javrasya]

Hi @JohnieBraaf. Thanks for reporting this and for the PR. I think this is a bug that should definitely be fixed probably in a similar way you did in your PR.

The thing is that the get_available_approvals method is an abstract method that both MSSQL and other DB versions are implementing. So we can't just update it for the MSSQL one.

The other thing is that this method is for fetching all available approvals for a user on every object but also can be used for a single workflow object. In such case I also think the object id should be passed down instead of applying the ORM filter on top of that (in here).

I will try to drop some comments on your changes. Let's try to discuss it there. Maybe even initialize a PR to have the conversation there.

[JohnieBraaf]

Thnx @javrasya for your feedback, I will have a look