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

Specify what should happen if Row.get(…) is called with a negative/out of bounds index/non-existing column name #184

Closed
mp911de opened this issue Oct 2, 2020 · 9 comments · Fixed by #240
Labels
target: 0.9.x Issues we want to address with 0.9.x. type: task A general task
Milestone

Comments

@mp911de
Copy link
Member

mp911de commented Oct 2, 2020

Right now, we're lacking a specification for Row.get(…) what should happen if the column index is out of bounds (negative, greater than the number of available columns) or a column name is used that isn't present in the result.

Analyzing the drivers yields the following behavior:

  • MySQL: NoSuchElementException/ArrayIndexOutOfBoundsException
  • Postgres: IllegalArgumentException
  • SQL Server: IllegalArgumentException
  • MariaDB: IllegalArgumentException
  • Spanner: IllegalArgumentException
  • H2: IllegalArgumentException

This ticket is to discuss the options and to agree on how implementations of Row.get(…) should behave.

@mp911de mp911de added for: team-attention An issue we need to discuss as a team to make progress type: task A general task labels Oct 2, 2020
@mp911de mp911de added the target: 0.9.x Issues we want to address with 0.9.x. label Apr 9, 2021
@mp911de
Copy link
Member Author

mp911de commented Jul 2, 2021

After discussing the ticket in the monthly call, we want to do something about this. The design choices range from a R2dbcException subtype to the use of built-in Java exceptions.

@elefeint
Copy link
Contributor

elefeint commented Jul 2, 2021

I just wanted to make sure I did not mislead in the meeting -- I think IllegalArgumentException is semantically perfectly appropriate; I was not trying to tip the scales towards a custom exception.

@mp911de
Copy link
Member Author

mp911de commented Jul 2, 2021

All good, these are all valid options.

@Michael-A-McMahon
Copy link
Contributor

I agree that IllegalArgumentException seems like the best choice, as it is one of the exception types listed in section 11 of the 0.8.x specification. Drivers should follow the spec.

When invoking Row.get(...) with an invalid index or name, IllegalArgumentException seems more descriptive than IllegalStateException or UnsupportedOperationException. R2dbcException would signify a failed database interaction, so is not applicable here.

@lukaseder
Copy link
Contributor

R2dbcException would signify a failed database interaction, so is not applicable here.

Well, the JDBC precedent is ResultSet.getXYZ() throws SQLException...

@mp911de
Copy link
Member Author

mp911de commented Jul 2, 2021

The problem with JDBC is that everything is an SQLException and drivers do not consistently implement the entire exception hierarchy.

@lukaseder
Copy link
Contributor

and drivers do not consistently implement the entire exception hierarchy.

Same as here, given your initial issue text? 😀

@mp911de
Copy link
Member Author

mp911de commented Jul 2, 2021

Right now, the error behavior isn't specified so everyone is doing something different and now we have the chance to clean up things and actually put them into the TCK to help implementors convince towards the spec.

@mp911de
Copy link
Member Author

mp911de commented Sep 2, 2021

We make already use of IndexOutOfBoundsException in Statement.bind(…). It would make sense to follow the line of thought and use the same exception in Readable.get and metadata classes. Also make use of NoSuchElementException as a more descriptive exception for get(String), getColumnMetadata(String) and other places. We don't need to introduce new exceptions, rather we use what the Java standard libraries provide.

@mp911de mp911de added this to the 0.9.0.RC1 milestone Sep 2, 2021
@mp911de mp911de closed this as completed in f55bb5d Sep 9, 2021
@mp911de mp911de removed the for: team-attention An issue we need to discuss as a team to make progress label Sep 9, 2021
mp911de added a commit to pgjdbc/r2dbc-postgresql that referenced this issue Oct 18, 2021
…fication.

We now throw the correct exception type according to the spec.

[resolves #456][r2dbc/r2dbc-spi#184]

Signed-off-by: Mark Paluch <mpaluch@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target: 0.9.x Issues we want to address with 0.9.x. type: task A general task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants