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

PgServerException #1904

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

PgServerException #1904

wants to merge 1 commit into from

Conversation

jorsol
Copy link
Member

@jorsol jorsol commented Sep 30, 2020

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does ./gradlew autostyleCheck checkstyleAll pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

@davecramer
Copy link
Member

What is this fixing ?

@jorsol
Copy link
Member Author

jorsol commented Sep 30, 2020

Is a draft to fix #963, it's a breaking change as I said in the comment of that issue, just to show how to implement exception hierarchy and also improve (IMHO) how to throw a server exception and also include a complete set of SQL status code from PostgreSQL.

This is how to get the server exception here:

try (Statement stmt = con.createStatement()) {
    stmt.executeQuery("select foo()");
    fail("function foo() does not exist");
 } catch (SQLSyntaxErrorException sqle) {
    if (sqle.getCause() instanceof PgServerException) {
        PgServerException pgex = (PgServerException) sqle.getCause();
        String message = pgex.getMessage();
        String sqlstatus = pgex.getSQLState();
        String formatedMessage = pgex.toString();
        String hint = pgex.getHint();
        int pos = pgex.getPosition();
    }
 }

If this is of interest I could keep working on this as time allows me, if not, we can just close it.

@davecramer
Copy link
Member

So I think this is OK to continue working on for 42.3.x

@jorsol
Copy link
Member Author

jorsol commented Nov 17, 2020

So I think this is OK to continue working on for 42.3.x

Sorry I haven't found time to work on this. If the intention is to make 42.3.x a breaking change, then this should definitely need to go on 43.3.0 as a breaking change, to make this less invasive the original exceptions could be deprecated instead of removed, but that doesn't make sense since there are not used anymore and would silently be ignored by clients.

Is there any deadline for pushing this to 42.3? I mean, I also want some feedback if this is worth it.

@davecramer
Copy link
Member

So I think this is OK to continue working on for 42.3.x

Sorry I haven't found time to work on this. If the intention is to make 42.3.x a breaking change, then this should definitely need to go on 43.3.0 as a breaking change, to make this less invasive the original exceptions could be deprecated instead of removed, but that doesn't make sense since there are not used anymore and would silently be ignored by clients.

Is there any deadline for pushing this to 42.3? I mean, I also want some feedback if this is worth it.

There is no deadline per-se, however I would like to get it pushed as we are now supporting two branches and I would rather only support one

@jorsol jorsol added this to In progress in 42.3.0 Release via automation Dec 30, 2020
@jorsol jorsol added this to the 42.3.0 milestone Dec 30, 2020
@jorsol jorsol self-assigned this Dec 30, 2020
@bokken
Copy link
Member

bokken commented Dec 31, 2020

This certainly is not passive, but it is a great improvement to be able to use the sql exception hierarchy.

@jorsol jorsol added release/breaking-change Breaking change that must be highlighted on the release notes release/noteworthy-feature Fix or feature that must be highlighted on the release notes labels Feb 14, 2021
@jorsol jorsol modified the milestones: 42.3.0, 43.0.0 Mar 10, 2021
@jorsol jorsol removed this from In progress in 42.3.0 Release Sep 28, 2021
@sheluchin
Copy link

sheluchin commented Sep 20, 2022

Is this still under consideration? It would be very helpful to be able to access the JDBC exception hierarchy instead of having to use e.getSQLState() + e.getNextException() to get the exception information.

@davecramer
Copy link
Member

Well clearly it would need to be rebased. @jorsol are you able to work on this ?

@jorsol
Copy link
Member Author

jorsol commented Sep 20, 2022

Well clearly it would need to be rebased. @jorsol are you able to work on this ?

Please note that currently, this will be a breaking change for those that use or depend on PSQLException, maybe this weekend I could probably find some time to rebase this, but it should not be taken lightly... probably I can make it less "breaking" by deprecating PSQLException first, yet some tools/apps that use that will still need to be updated.

@davecramer
Copy link
Member

Well clearly it would need to be rebased. @jorsol are you able to work on this ?

Please note that currently, this will be a breaking change for those that use or depend on PSQLException, maybe this weekend I could probably find some time to rebase this, but it should not be taken lightly... probably I can make it less "breaking" by deprecating PSQLException first, yet some tools/apps that use that will still need to be updated.

Good point! We can add a property to throw the new exception and then eventually deprecate that?

@jorsol
Copy link
Member Author

jorsol commented Sep 20, 2022

Good point! We can add a property to throw the new exception and then eventually deprecate that?

Well, that's a good idea, a property might do the trick, yet I have been a bit busy so, please give me a couple of months to work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-incompatible compat-jdbc-spec release/breaking-change Breaking change that must be highlighted on the release notes release/noteworthy-feature Fix or feature that must be highlighted on the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants