Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Oracle Warning Segments #98

Merged
merged 5 commits into from
Oct 10, 2022
Merged

Oracle Warning Segments #98

merged 5 commits into from
Oct 10, 2022

Conversation

Michael-A-McMahon
Copy link
Member

This branch adds a new Result.Segment type for database warnings. The new type is intended for consumption by user code, so it is added as a public type in the oracle.r2dbc package: OracleR2dbcWarning.

In the previous release, warnings were emitted as Result.Message segments. Message segments cause onError signals to be emitted when consuming results of SQL calls. Based on discussions with several users, I'm convinced that this behavior was not correct.
In the case of a warning, an onError signal is not helpful as it prevents user code from consuming the actual result of the SQL call. For instance, a call to Result.map(Function) would result in an error even if rows were successfully queried, but a warning was raised.

With this branch, warnings are emitted as a distinct message type. User code may choose to consume the warning if desired, but otherwise the warning will be ignored. This model is inspired by the JDBC API, which does not throw SQLWarning exceptions, but instead requires an explicit call to a getWarnings method.

Although it is similar to Result.Message, I choose to make OracleR2dbcWarning a distinct type. The intent of this is to avoid confusion for driver agnostic code which may (rightfully so) consider all Message segments to be errors. Driver agnostic code can then remain agnostic, and not need to reference a type that is specific to Oracle R2DBC in order to discern between errors and warnings.

Thanks to @larousso and @rathoreamrsingh for providing helpful discussions that lead to this change.

@Michael-A-McMahon
Copy link
Member Author

I should look into the test action failure before we merge this. But I'm done for today, so it will have to wait until tomorrow.

* }</pre>
* @since 1.1.0
*/
public interface OracleR2dbcWarning extends Result.Segment {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extending Result.Message would likely make more sense to pick up what's already provided by the spec.

Copy link
Member Author

@Michael-A-McMahon Michael-A-McMahon Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really appreciate you joining in on this!

I can see how a Message subtype is much easier to work with, and I'm happy to make that change.

Would you be open to adding an instanceof OracleR2dbcWarning check in Spring then? Users are telling me that they don't want warnings to be emitted as onError signals. So if Spring has an instanceof Message check that results in onError, we'd want to avoid taking that branch.

The main goal is to make warnings an "opt-in" type of result. That is, code which consumes a Result should not see a warning unless it explicitly checks for it. It is similar to JDBC's getWarnings methods, where code won't see the warning unless it checks. To this end, calls to Result.getRowsUpdated or Result.map methods would not emit onError for warnings, even if the segment type is a Result.Message subtype.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that emitting warnings as errors creates a lot of inconvenience in the first place.

Users are telling me that they don't want warnings to be emitted as onError signals.

I also understand the desire to inform users that something unexpected has happened that didn't immediately lead to failure. Most other drivers do not emit warnings as errors but rather allow consumption of warning (message) segments when users are interested in such detail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying on this; Knowing that other drivers handle it this way is very helpful!
I've made the suggested changes.

@jeandelavarene jeandelavarene merged commit aada73f into main Oct 10, 2022
@jeandelavarene jeandelavarene deleted the 93-oracle-warning branch October 10, 2022 18:14
@rathoreamrsingh
Copy link

Hi @Michael-A-McMahon
May I know this fix will be available in which release of R2DBC?
And will there be any release for the library based on 0.4.0 release?

Thanks

@Michael-A-McMahon
Copy link
Member Author

We aim for quarterly releases, so we are already overdue for a 1.1.0 release. The team is working to make that available very soon, but I can't commit to a date just yet.

I know that a lot of programmers would like us to patch the 0.4.0 release, as it is currently the only one compatible with Spring. However, as time moves forward, a new release of Spring will be compatible with our 1.x releases. As a future investment, I want to sink all of my development effort into the 1.x releases.

I have no plan to update the older releases for now, as it would detract from the time I can put into development of newer releases. I know this not an ideal situation at the moment, but I think it will pay off when a new Spring release supports the 1.0.0 R2DBC SPI.

@Michael-A-McMahon Michael-A-McMahon linked an issue Nov 10, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants