chore: Remove fixed or addressed TODOs #2551
Conversation
| // TODO check for all `throw SQLException` in the code? | ||
| // TODO could some of them replaced wit other errors? |
There was a problem hiding this comment.
No usage of SQLException or ExposedSQLException in core. Only in exposed-jdbc
There was a problem hiding this comment.
should we use SQL Exception instead of error?
There was a problem hiding this comment.
@e5l I would hesitate to do this for 3 reasons:
java.sql.SQLExceptionseems very JDBC-specific. It never comes up as part of the R2DBC spi. I at least looked through H2+PG r2dbc repos and they never throw this exception (they only catch and re-purpose). So it feels odd to use it inexposed-corenow with driver separationTransactionManagerlogic has different behavior based on type of exception caught.SQLExceptionis always caught first (inexposed-jdbc) and triggers (potentially) more attempts to execute the same statement, whereas an ISE thrown by Exposed will not trigger multiple runs.- There's currently no instance of Exposed throwing such an exception; we catch any thrown naturally by the DB and wrap as
ExposedSQLException. So currently there's a clear distinction between exceptions thrown by DB and exceptions thrown by Exposed, but if we started mimicking DBSQLException, wouldn't that blur?
| // TODO Discuss keeping this in core (not used here) & package mismatch | ||
| // Consider changing package if moving to jdbc module |
There was a problem hiding this comment.
More details in Slack, if this needs to be reconsidered
| // TODO should we allow users to use ResultApi manually with possibility to call next()/getObject() in the code? | ||
| // TODO Or the `mapRows` is the only right way to use it? |
There was a problem hiding this comment.
If you expand down, JdbcResult at some point was extended to provide next() functionality and already implements RowApi.getObject(). So users do have a choice to manually iterate over results or use mapRows().
| User.name like stringLiteral("Hich%").upperCase() | ||
| }.map { it[User.name] } | ||
| .toList() // TODO this is the only unmapped Iterable <-> Flow operation; impossible? | ||
| .toList() |
There was a problem hiding this comment.
The goal of this TODO was to have a seamless duplicate of the JDBC test, in the event they could be merged or reused to a certain degree in the future, meaning the ability to call Flow<T>.map { } and return List<T> without always needing to chain .toList().
I tried many overload variations, dummy parameters, and even kotlin.internal.HidesMembers, and could not avoid getting an overload ambiguity resolution error.
If there are any suggestions, or preference to keep this, let me know.
| compileOnly(libs.postgre) | ||
| compileOnly(libs.r2dbc.postgresql) | ||
| } | ||
| // TODO confirm use of repomix.config.json +/- remove? |
There was a problem hiding this comment.
@e5l If I recall correctly, you mentioned that the file exposed-r2dbc/repomix.config.json was added for Junie?
But it was only added to that single module, not to any others?
Please confirm again whether the file should be left alone or removed.
exposed-r2dbc/src/main/kotlin/org/jetbrains/exposed/v1/r2dbc/statements/R2dbcConnectionImpl.kt
Show resolved
Hide resolved
| // Todo relies on rowsUpdated throwing ISE if already consumed; should we fail earlier as in mapRows()? | ||
| /** Returns a Flow containing the number of rows updated by executing a statement. */ | ||
| /** | ||
| * Returns a Flow containing the number of rows updated by executing a statement. | ||
| * | ||
| * @throws IllegalStateException If the result has already been consumed by another flow operation. | ||
| */ |
There was a problem hiding this comment.
I ran tests to confirm the behavior and found the following:
- If a statement that is meant to update rows is executed, like
INSERT RETURNING, and the result is consumed first bymapRows(), then SPIrowsUpdatedthrows ISE. - If a
SELECTstatement is executed and the result is consumed withmapRows(), then usingrowsUpdateddoesn't throw; it will just return an empty flow. Which makes sense I guess because no rows were meant to be updated.
So I'm hesitant to have us fail early instead, only based on whether mapRows() was called or not, given that the result would not be equal to the underlying behavior of the SPI rowsUpdated.
I'm leaving that decision to the SPI and just updated the KDocs instead.
| // TODO check for all `throw SQLException` in the code? | ||
| // TODO could some of them replaced wit other errors? |
There was a problem hiding this comment.
should we use SQL Exception instead of error?
- Drop unused repomix files from exposed-r2dbc
bog-walk
force-pushed
the
bog-walk/remove-fixed-todos
branch
from
bd03b90 to
d271bcd
Compare
Description
Summary of the change: Remove TODOs that have already been addressed or that don't require a fix.
Detailed description:
SQLExceptionin core already removed when V1 mergedExposedSQLException+ExposedR2dbcExceptionin Slack channelJdbcResultalready containsnext()and implementsRowApi.getObject()Flow.map()exposed-r2dbc?rowsUpdatedas throw behavior changes depending on whether result is emptyType of Change
Please mark the relevant options with an "X":