heissenberg06 commented on PR #6058: URL: https://github.com/apache/fineract/pull/6058#issuecomment-4847145959
Thanks @adamsaghy , that's a good challenge — I dug into the three justifications in the wrapSQL comment: 1. "prevent malicious sql": this no longer holds. The wrapping copies the inner SQL verbatim into the derived table, so it provides no injection protection by itself. Injection is handled separately by SQLInjectionValidator / the parameterized-query work in FINERACT-2081 and the column validators, not by wrapSQL. 2. "CachedRowSetImpl column-label bug" (Sun bug 7046875): the code reaches the result set via JdbcTemplate.queryForRowSet → SqlRowSetResultSetExtractor, which on JDK 21 obtains the CachedRowSet via javax.sql.rowset.RowSetProvider rather than the old com.sun.rowset.CachedRowSetImpl directly. So the original bug likely no longer applies, though this needs to be confirmed with a test (a query selecting AS-aliased columns returning correct labels without wrapping). 3. "prevent JDBC sql errors": there's no concrete reference for this in the comment; I'd want to find what concretely breaks (if anything) when the wrapping is removed. So I agree it's worth testing whether wrapSQL can simply be dropped. If it can, that removes the root cause and this ORDER BY issue disappears without the SQL-parsing logic in this PR. Proposed plan: I'll run the reporting tests with wrapSQL turned into a no-op to see what (if anything) fails — column-label handling and any dialect-specific behavior in particular. If it's clean, I'll repurpose this PR to remove the wrapping (and simplify the call sites) instead of adding the ORDER BY lifter. Does that direction work for you? Any specific cases (DB dialects, label edge cases) you'd want covered before removing it? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
