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: Revert inet to return as a string and handle values with net masks #1568

Merged
merged 1 commit into from Sep 13, 2019

Conversation

@sehrope
Copy link
Contributor

commented Sep 11, 2019

Reverts inet type columns to default to returning their value as the text representation returned by the backend rather than parsing them as InetAddress values. This ensures that the mask value is not lost.

Also corrects the getObject(..., InetAddress.class) handling to split out the net mask and return only the address portion.

I've also updated the tests for inet data types to clean it up a bit, explicitly test getString() / getObject(), and add some additional edge cases (ex: "10.20.30.40/32" gets round tripped by the server to strip out the /32).

When (if?) we change the default return type to give a PGInet that test should fail and need to be updated. As I tend to lean toward backwards compatibility, I think we should leave the default as a text and if anything provide PGInet as a separate class that anyone who wants can use. Could consider using something similar to handle mac data types too.

Closes #1567

See also #1134 for related discussion.

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 mvn checkstyle:check pass ?
  3. Have you added your new test classes to an existing test suite?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.

Yes. It reverts inet data type handling to return a String instead of InetAddress by default.

  • 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?
@sehrope

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

FYI I copied the error message state codes from the existing function so it's set to PSQLState.INVALID_PARAMETER_VALUE. Let me know if there's a better value for to use instead.

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Excellent. Thanks. I'm inclined to push this but am going to let it bake a bit to see if anyone else has any comments.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Sep 11, 2019

@sehrope

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Not sure what's up with the AppVeyor build. One out of four build configs failed but it's something unrelated to this PR: https://ci.appveyor.com/project/davecramer/pgjdbc/builds/27342998/job/b5394yla9yfc7aku#L479

@maffe

This comment has been minimized.

Copy link

commented Sep 11, 2019

Regarding backwards compatibility, previous to #1134/#1527 inet types were returned as PGobject, not String.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Sep 11, 2019

@sehrope

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@maffe Nice catch! Looks like that piece of the code is still in there:

if (type.equals("inet")) {
try {
return InetAddress.getByName(value);
} catch (UnknownHostException e) {
throw new PSQLException(GT.tr("IP address {0} of a host could not be determined", value),
PSQLState.CONNECTION_FAILURE, e);
}
}

I bet it's not being called as that's the fallback when the more specific getXyz(...) methods cannot figure out what to return. I'm going to try removing it and the string handling to restore the original behavior.

@sehrope sehrope force-pushed the sehrope:fix-inet-parsing branch from 2ae8a79 to 9814662 Sep 11, 2019
@sehrope

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Okay force pushed an update that reflects the actually prior behavior of returning a PGObject but maintains the fixes for getting values as an InetAddress when it has a mask.

… net masks

Reverts inet type columns to return their value as a PGObject rather than parsing
them as InetAddress values. This ensures that the mask value is not lost.

Also corrects the getObject(..., InetAddress.class) handling to split out the net mask
and return only the address portion.
@sehrope sehrope force-pushed the sehrope:fix-inet-parsing branch from 9814662 to e2fd2ce Sep 11, 2019
@AppVeyorBot

This comment has been minimized.

Copy link

commented Sep 11, 2019

@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

This actually works to our advantage in the case where we decide to return a custom object as it can always be cast to a PGObject. As long as we keep toString() the same we should be good.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Sep 11, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Sep 11, 2019

Codecov Report

Merging #1568 into master will decrease coverage by 3.69%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             master   #1568     +/-   ##
==========================================
- Coverage      68.9%   65.2%   -3.7%     
+ Complexity     3972    3889     -83     
==========================================
  Files           179     179             
  Lines         16565   16943    +378     
  Branches       2695    2768     +73     
==========================================
- Hits          11414   11048    -366     
- Misses         3899    4610    +711     
- Partials       1252    1285     +33
@davecramer

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Anyone see any reason not to push this ?

@davecramer davecramer merged commit 3df32f9 into pgjdbc:master Sep 13, 2019
3 checks passed
3 checks passed
codecov/project 68.91% (+0.01%) compared to e9423dc
Details
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
5 participants
You can’t perform that action at this time.