-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add support for Postgres inet type #413
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rausnitz Thanks for proposing this. SwiftNIO's SocketAddress
is a somewhat special type... It still has some rough C edges.
Besides that, accepting this patch would increase the shared API surface between PostgresNIO and SwiftNIO. However our long term goal is to make SwiftNIO an implementation detail of PostgresNIO. This would be a step in the opposite direction. Because of this I'm inclined to reject this PR. @gwynne WDYT?
@fabianfett Yeah, I don't love tying this to |
@rausnitz I think I should rephrase my concerns: If you implement a |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
==========================================
+ Coverage 49.19% 54.89% +5.70%
==========================================
Files 108 109 +1
Lines 8845 7939 -906
==========================================
+ Hits 4351 4358 +7
+ Misses 4494 3581 -913
|
@fabianfett Let me know if f972b92 is what you had in mind, and I can add tests if so. |
public struct PostgresINET { | ||
public let ipFamily: UInt8 | ||
public let netmaskLength: UInt8 | ||
public let isCIDR: Bool | ||
public let addressLength: UInt8 | ||
public let ipAddress: [UInt8] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is too close to the on the wire representation. In an ideal case this would be closer to the semantical meaning:
struct PostgresIPv4 {
var value: (UInt8, UInt8, UInt8, UInt8)
}
struct PostgresIPv6 {
var value: (UInt16, UInt16, UInt16, UInt16, UInt16, UInt16, UInt16, UInt16)
}
enum PostgresINET {
case ipv4(PostgresIPv4, networkMask: UInt8?)
case ipv4(PostgresIPv6, networkMask: UInt32?)
}
Since there are no currency types for this in Swift, we need to have our own minimal type for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabianfett Without an explicit isCIDR
property, would it be safe to infer that an instance uses CIDR notation based on whether the IPv4 network mask is 32 (or IPv6 network mask is 128)?
We implemented this in our project so we could use the Postgres inet type with PostgresNIO. Postgres's inet and SwiftNIO's
SocketAddress
aren't a perfect match but they're fairly close.Let me know if you'd like to have this be a part of PostgresNIO. I also have tests I can port over from our project.