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

Allow mapping inet as (IPAddress, int). #2698

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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