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

Add support for Postgres inet type #413

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rausnitz
Copy link
Sponsor Contributor

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.

Copy link
Collaborator

@fabianfett fabianfett left a 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?

@rausnitz
Copy link
Sponsor Contributor Author

@fabianfett Yeah, I don't love tying this to SocketAddress here either, though it's nice as a shortcut in our particular project. I stil wonder about the rest of it: Does PostgresNIO have an interest in providing a way to parse the binary representation of types that don't have good Swift counterparts? It seems like a waste that others would have to re-write the code for handling inet data, but I'm not sure what is the place for that in PostgresNIO outside of a PostgresCodable implementation.

@fabianfett
Copy link
Collaborator

@rausnitz I think I should rephrase my concerns:

If you implement a PostgresINET type that represents what the Postgres inet type does without depending on NIO, I'll be happy to merge that PR. Does that make sense?

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2023

Codecov Report

Merging #413 (f972b92) into main (92ee156) will increase coverage by 5.70%.
Report is 6 commits behind head on main.
The diff coverage is 0.00%.

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     
Files Coverage Δ
...resNIO/New/Data/PostgresINET+PostgresCodable.swift 0.00% <0.00%> (ø)

... and 6 files with indirect coverage changes

@rausnitz
Copy link
Sponsor Contributor Author

If you implement a PostgresINET type that represents what the Postgres inet type does without depending on NIO, I'll be happy to merge that PR. Does that make sense?

@fabianfett Let me know if f972b92 is what you had in mind, and I can add tests if so.

Comment on lines +1 to +7
public struct PostgresINET {
public let ipFamily: UInt8
public let netmaskLength: UInt8
public let isCIDR: Bool
public let addressLength: UInt8
public let ipAddress: [UInt8]
}
Copy link
Collaborator

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.

Copy link
Sponsor Contributor Author

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)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants