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

fix: Update error message for COPY commands executed using JDBC API #1300

Merged
merged 1 commit into from Nov 13, 2019

Conversation

@sehrope
Copy link
Contributor

sehrope commented Sep 17, 2018

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?

This updates the error message for COPY to STDIN / STDOUT commands that are not executed using the CopyManager API, i.e. via a regular Statement.executeQuery("COPY ... TO STDOUT ..."). The revised text for the error message is:

 COPY commands are only supported using the CopyManager API.

If anybody has a better suggestion feel free to chime in. I figure it's enough that someone who does run into the error can then search for "CopyManager" in the context of pgjdbc and hopefully find more info.

This PR does not update or remove the old value from the translation files. There's quite a few translations for the old message as it was in the code base for many years. I was considering updating the old message key for the old text so they continue to operate but decided that it's better to have the new / correct english text show up instead.

Separately, we should look into adding more about the CopyManager interface to either the core README or the jdbc.postgresql.org website as I don't think either covers that API.

@sehrope sehrope mentioned this pull request Sep 17, 2018
1 of 2 tasks complete
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 17, 2018

Codecov Report

Merging #1300 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1300      +/-   ##
============================================
- Coverage     68.74%   68.73%   -0.02%     
+ Complexity     3903     3901       -2     
============================================
  Files           179      179              
  Lines         16414    16413       -1     
  Branches       2672     2672              
============================================
- Hits          11284    11281       -3     
- Misses         3883     3884       +1     
- Partials       1247     1248       +1
@@ -71,6 +71,7 @@
public class QueryExecutorImpl extends QueryExecutorBase {

private static final Logger LOGGER = Logger.getLogger(QueryExecutorImpl.class.getName());
private static final String COPY_ERROR_MESSAGE = "COPY commands are only supported using the CopyManager API.";

This comment has been minimized.

Copy link
@vlsi

vlsi Sep 17, 2018

Member

That does not make the thing much cleaner. What do you think if we add a relevant URL to the documentation?

This comment has been minimized.

Copy link
@jorsol

jorsol Sep 17, 2018

Contributor

While right now the documentation is pretty much static on URL definition, but in the future there is a risk that a refactor to the documentation change the links...

And BTW I don't see the documentation for the CopyManager API in the index of the documentation.

This comment has been minimized.

Copy link
@sehrope

sehrope Sep 17, 2018

Author Contributor

I don't think we should have direct URLs in the source code. Even with control of the host it's a pain if you ever decide to change things as there will be lingering references in the wild.

The error message should be enough that someone with a couple minutes of online searching could find. The first Google search result for "pgjdbc CopyManager" is the javadoc so I think that's good enough for now.

FYI, my last comment in the PR was referring to having some documentation about CopyManager in the docs. AFAICT there is nothing to which we could link in the docs even if we wanted to. At best it's be linking to a CopyManager example in the tests or the javadoc.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Sep 17, 2018

@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Jan 29, 2019

Somewhat related to this, any thoughts on having a more specific PSQLException subclass thrown for this situation? That way if a user is checking for the specific error they don't have to deal with future changes in the specific messaging.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Jan 29, 2019

@sehrope I guess I was over eager on the URL thing.
+1 to a more specific Exception.

@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Jan 29, 2019

Okay I'll update this PR to reflect that. Any suggestions for the exception name? Was thinking CopyUnsupportedException with a message text of: "COPY commands are only supported using the CopyManager API."

Could make the exception name a bit more specific instead, say NonCopyManagerUnsupportedCopyException though that seems a bit long.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Jan 29, 2019

@sehrope in lieu of anything more brilliant that is fine

@sehrope sehrope force-pushed the sehrope:copy-error-message branch 2 times, most recently from c46f6a0 to 8e85084 Jan 30, 2019
@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Jan 30, 2019

Rebased atop master and adds a commit to throw a new PSQL subclass, CopyNotSupportedException, for COPY commands that are executed via non-CopyManager API. The tests have also been updated to catch those specific exceptions.

Because of how the driver manages connection state, the COPY error message round trips from the client to and back from the server before being thrown as a Java exception. To get the custom exception to be thrown, the receiveErrorResponse() has been updated to check for that specific error message. If there's a better way to do it I'm open to ideas but seemed like the most direct approach.

@@ -2437,7 +2438,13 @@ private SQLException receiveErrorResponse() throws IOException {
LOGGER.log(Level.FINEST, " <=BE ErrorMessage({0})", errorMsg.toString());
}

PSQLException error = new PSQLException(errorMsg);
PSQLException error;
if (errorMsg.getMessage() != null && errorMsg.getMessage().endsWith(COPY_ERROR_MESSAGE)) {

This comment has been minimized.

Copy link
@jorsol

jorsol Jan 30, 2019

Contributor

Assuming there is a translated string, will this work?
This will also check for that string for every PSQLException throw, which doesn't look good from a perspective that CopyNotSupportedException might be throw in a 1% of use cases?

This comment has been minimized.

Copy link
@sehrope

sehrope Jan 30, 2019

Author Contributor

I'm not sure about the translated string case. There's no translation now but I think you're right about it not working in that situation as the translated one would end up being round tripped.

As this is only when dealing with errors doubt there's a measurable perf difference for normal usage. Still if there's a way to track the error with a connection level state variable (say a boolean to track the most recent error is COPY related) that'd handle both that and the translation issue.


package org.postgresql.util;

public class CopyNotSupportedException extends PSQLException {

This comment has been minimized.

Copy link
@bokken

bokken Feb 13, 2019

Member

exceptions are serializable. should we generate a serial version id?
It looks like PSQLException does not have one declared either...

@sehrope sehrope force-pushed the sehrope:copy-error-message branch from 8e85084 to e7d2be7 Feb 27, 2019
@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Feb 27, 2019

I've removed the last commit that adds the custom exception class. While it'd be nice to have, it's going to take a bit more work on both ends of the protocol (client side or server side generation). No point in holding up the improved COPY error message until we figure out how to deal with that.

@sehrope sehrope force-pushed the sehrope:copy-error-message branch from e7d2be7 to f7e543c Feb 27, 2019
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 27, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Feb 27, 2019

@jorsol
jorsol approved these changes Feb 27, 2019
@sehrope sehrope force-pushed the sehrope:copy-error-message branch from f7e543c to ae2df44 Apr 6, 2019
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Apr 6, 2019

@sehrope sehrope force-pushed the sehrope:copy-error-message branch from ae2df44 to 12ea295 Nov 13, 2019
@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Nov 13, 2019

@davecramer I rebased this and might as well merge it in as well. It fixes the "COPY is not supported" error message to instead suggest using the CopyManger API.

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Nov 13, 2019

@davecramer davecramer merged commit c99ed12 into pgjdbc:master Nov 13, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.