Skip to content

Conversation

@PJB3005
Copy link

@PJB3005 PJB3005 commented Mar 19, 2023

Fixes #1158

Unified cidr and inet handling code since they're similar.

I have no idea what I'm doing and barely understand how this code works, so please review carefully.

Fixes npgsql#1158

Unified cidr and inet handling code since they're similar.
@roji
Copy link
Member

roji commented Mar 20, 2023

@PJB3005 this looks pretty good!

However... I've been thinking of getting rid of the type mapping to a value tuple for a while, and instead introducing a real struct type. I've opened npgsql/npgsql#5004 to do this at the ADO.NET layer; once that's done, we can also add the mapping here, as you've done.

How does that sound?

@PJB3005
Copy link
Author

PJB3005 commented Mar 20, 2023

I agree that would probably be a better idea, though you'd still need to keep it around for cidr thanks to backwards compatibility.

Also, just so I understand how everything fits together here correctly. The code that turns inet -> (IPAddress, int) is in the ADO.NET layer, and the type mapping code in this PR is more telling EF Core "this is a valid conversion by the way"?

@roji
Copy link
Member

roji commented Mar 20, 2023

I agree that would probably be a better idea, though you'd still need to keep it around for cidr thanks to backwards compatibility.

If we do this, this would be a breaking change for both cidr and inet (both support the value tuple). Unfortunately we can't obsolete this feature before removing it - which would have been better - since this isn't a specific API or type we can slap [Obsolete] on - it's a generic type argument we support on NpgsqlDataReader.GetFieldValue. However, I've very rarely seen anyone use this particular feature, so I don't think it would be a big problem.

Also, just so I understand how everything fits together here correctly. The code that turns inet -> (IPAddress, int) is in the ADO.NET layer, and the type mapping code in this PR is more telling EF Core "this is a valid conversion by the way"?

Pretty much. EF's TypeMappingSource is what defines what types are supported at the ADO.NET layer, and which CLR types map to which (string) store types. The type mappings it resolves also provide some extra services, like rendering a SQL literal for a given value, so we can embed literals in e.g. queries.

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.

Unable to map inet column to (IPAddress, int) tuple

2 participants