AshharAhmadKhan commented on PR #6494:
URL: https://github.com/apache/fineract/pull/6494#issuecomment-6592098449
> > 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-sta
tement 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?
Definitely adam, thanks for the review.
--
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]
