adamsaghy commented on PR #6494:
URL: https://github.com/apache/fineract/pull/6494#issuecomment-6591802277
> Thank you @galovics . @adamsaghy please let me know your thoughts on this
as well.
Thanks @galovics, good catch. I looked into it and agree on the direction,
but @Transactional(readOnly = true) alone won’t close this:
The annotation has no effect on the primary transaction manager. The
@Primary one is ExtendedJpaTransactionManager. With EclipseLink, Spring doesn’t
set read-only on the JDBC connection, so readOnly = true only changes the flush
mode. We’d need to run the report through jdbcTransactionManager and send SET
TRANSACTION READ ONLY ourselves as the first statement.
ExtendedDataSourceTransactionManager only turns on enforceReadOnly when the
instance is in read-only mode. Sending the statement ourselves also settles
your MariaDB question, since we wouldn’t depend on the driver passing
setReadOnly on. MariaDB, MySQL and PostgreSQL all accept it at the start of a
transaction.
On PostgreSQL, a read-only transaction can be escaped with several
statements. pgjdbc splits on ;, so SELECT 1; COMMIT; DELETE FROM m_loan ends
the read-only transaction and runs the DELETE in a normal one.
set_config('default_transaction_read_only', 'off', ...) works the same way. So
we also need a single-statement check. That doesn’t mean writing our own lexer:
org.postgresql.core.Parser.parseJdbcSql(...) returns one NativeQuery per
statement with its command type. Requiring exactly one SELECT/WITH statement
uses the same splitting the driver applies. MariaDB and MySQL don’t need this,
because allowMultiQueries is off by default and MariaDB has no data-modifying
CTEs.
Also, the wrapper never fully protected PostgreSQL. Report SQL like SELECT
1) x; DELETE FROM m_loan; SELECT * FROM (SELECT 1 becomes three valid
statements once wrapped. This PR doesn’t open a new hole; the gap is already on
develop.
So I’d merge this as is and handle it in a follow-up ticket:
run report SQL in a JDBC read-only transaction with an explicit SET
TRANSACTION READ ONLY
on PostgreSQL, reject report SQL that pgjdbc’s parser splits into more than
one statement
tests: COMMIT; chaining and a DML CTE on PostgreSQL, and a function that
writes, called from a SELECT, on MariaDB
longer term: let report execution use a connection whose database user only
has SELECT grants (the tenant’s readonly_schema_* settings could be reused),
which nothing in the SQL can get around
@AshharAhmadKhan, would you like to take the follow-up?
--
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]
