flowable / flowable-engine

A compact and highly efficient workflow and Business Process Management (BPM) platform for developers, system admins and business users.
https://www.flowable.org
Apache License 2.0
8.01k stars 2.63k forks source link

Fix order by clause missing "order by" #3853

Closed el-hackerino closed 3 months ago

el-hackerino commented 9 months ago

The generated ORDER BY clause is incorrect, leading to syntax errors in the resulting query. The ELSE branch in lines 64-55 did it correctly (but I refactored the code to combine the code).

I'm not set up to add/run tests for this, but I think this change should be looked at ASAP since the bug breaks a lot of queries.

Check List:

filiphr commented 8 months ago

@el-hackerino can you please provide an example query where this is causing issues? This code has been like this since 2017 and we have not received any issues about it.

Perhaps you are using it incorrectly. How are you passing the orderBy parameter?

el-hackerino commented 7 months ago

We're using a NativeQuery and passing the orderBy parameter. In Flowable 6.5, everything worked when we passed something like "CREATETIME ASC", resulting in a query starting with

SELECT SUB.* FROM ( select RES.* , row_number() over (order by RES.CREATE_TIME_ ASC ) rnk

Since Flowable 7.0, the result is

SELECT RES.* FROM ( select RES.*, row_number() over (RES.CREATE_TIME_ ASC ) as rnk 

because of this commit changing

order by ${orderByColumns}

into

${orderByForWindow}

in db2.properties.

The new orderByWindow value is generated either with order by included when there is no value and the default ID sorting is used or WITHOUT order by if a value is passed (see my suggested changes). The latter case produces a syntax error.

filiphr commented 7 months ago

@el-hackerino can you please share the java code you are using for this?

filiphr commented 7 months ago

@el-hackerino I've added a test case for this and I'll merge it once the tests pass.