Map network type operations #407
Conversation
|
@austindrenski, I know this PR is still in progress, but here's one thought... Unlike with the range operators, I think the inet operations shouldn't exist as a set of extension methods over The reason for this different is that |
austindrenski
force-pushed
the
network-address-operators
branch
from
600c890
to
8e654d3
Compare
Implementation considerations@roji From looking at the basic operators, it seems feasible to provide local implementations. My thought process was that if local implementations can be provided, and if those implementations are consistent with PostgreSQL, then extension methods would make sense. On the technical side, I'm hoping that PostgreSQL network address operations will be easier to replicate than range operators. I could be wrong here. But as in #323, inconsistent local implementations would be worse than always-throw implementations. Thinking about API consistency, I planned to follow the same implementation for What do you think? Do you foresee complications to the extension methods even if they support local evaluation?
|
austindrenski
force-pushed
the
network-address-operators
branch
from
8e654d3
to
f87d862
Compare
I'm still very wary of adding extensions methods on standard, non-database-related types in an EF Core provider... A good example is the (if you're interested we had a conversation about this with the EF Core team, I can dig it up).
I agree... I tend towards throwing in general, since IMHO there isn't a lot of value in the client implementation, and we have to be sure to provide a 100% accurate (and reasonably efficient) implementation - I'm not sure this is really worth our time. At the end of the day it's really up to you, but I'd recommend against it.
It's a bit of an oversight. Right now the so-called "provider-specific types" (NpgsqlInet, NpgsqlDateTime and the others) really supported in the EF Core provider - they're quite rarely-used. In most cases they only exist to provide access to the full range of precision/scale of the PostgreSQL type where the CLR type is more limited (e.g. PostgreSQL Long story short, yes, I think it makes sense to have the mappings as follows:
|
austindrenski
force-pushed
the
network-address-operators
branch
from
f87d862
to
9dbc029
Compare
To throw, or not to throw...
Ahhh. That's a great point, and a helpful way to think about feature scope as a new contributor. Perhaps that could be included in the refreshed documentation as the start of a contribution guide? I'll take a look at the pattern for Type mapping
This may be a silly question, but instead of default mapping It feels complicated to have one network type ( Could something like this work?
Object slicingOn a related note, does mapping |
I really like this idea. NpgsqlInet is almost the equivalent of an IPAdress/int tuple (it's a struct), it was created way before tuples existed. The only catch is that this would need to be done at the ADO.NET level first and foremost. That's no big deal, I can do that soon, and at that point you can submit a PR for the corresponding changes in EF Core. Sounds good?
You mean because PostgreSQL |
Type mapping
That works for me!
I agree that it would impose a burden on the common IP use... But it does feel like the makings of a footgun situation... Would it introduce too much overhead to log some type of warning if this occurs during materialization? For example, if the materialization is mapping If that's too much overhead (in general or relative to the risk), perhaps we can highlight the risk in the docs? Question on parameter reuse and duplicationI'm seeing parameter duplication when the same Is this expected behavior? Related to value types being copied on closure? Something else entirely? Example: [Fact]
public void Demonstrate_ValueTypeParametersAreDuplicated()
{
using (NetContext context = Fixture.CreateContext())
{
NpgsqlInet npgsqlInet = new IPAddress(0);
bool[] _ =
context.NetTestEntities
.Where(x => EF.Functions.ContainsOrEquals(x.CidrMappedToNpgsqlInet, npgsqlInet))
.Select(x => x.CidrMappedToNpgsqlInet.Equals(npgsqlInet))
.ToArray();
AssertContainsSql("SELECT x.\"CidrMappedToNpgsqlInet\" = @__npgsqlInet_0");
AssertContainsSql("WHERE x.\"CidrMappedToNpgsqlInet\" >>= @__npgsqlInet_0");
}
}Expectation: SELECT x."CidrMappedToNpgsqlInet" = @__npgsqlInet_0
FROM "NetTestEntities" AS x
WHERE x."CidrMappedToNpgsqlInet" >>= @__npgsqlInet_0 = TRUEReality: SELECT CASE
WHEN x."CidrMappedToNpgsqlInet" = @__npgsqlInet_1
THEN TRUE::bool ELSE FALSE::bool
END
FROM "NetTestEntities" AS x
WHERE x."CidrMappedToNpgsqlInet" >>= @__npgsqlInet_0 = TRUE |
austindrenski
force-pushed
the
network-address-operators
branch
from
7d22d08
to
5394545
Compare
…xtensions to match NpgsqlDbFunctionsExtensions. Changed network operation extension methods to use DbFunctions instead of IPAddress/NpgsqlInet per the discussion in npgsql#407. Added XML docs to indicate that all of these extension methods throw a NotSupportedException outside of EF Core.
|
See #1937 for a PR for your changes at the ADO level, let me know what you think.
Regarding the data loss/truncation problem, I don't think there's actually a problem. The PostgreSQL docs say that "The inet type holds an IPv4 or IPv6 host address, and optionally its subnet". This is also shown by the fact that PostgreSQL doesn't force you to specify a subnet in the From the .NET perspective, it's a mater of what the user tells us they want. They always the option of saying they want the subnet - this is done by requesting an Note that this is very different from precision truncation, where, say, a database double loses precision when read... This is a column containing two distinct pieces of information, one of which is totally optional. So I think it's OK (maybe @YohDeadfall has an opinion). I don't have time at the moment to look at the parameter reuse/duplication issue above, I'll try to do so soon. |
austindrenski
force-pushed
the
network-address-operators
branch
from
7254cbf
to
9120e0e
Compare
…xtensions to match NpgsqlDbFunctionsExtensions. Changed network operation extension methods to use DbFunctions instead of IPAddress/NpgsqlInet per the discussion in npgsql#407. Added XML docs to indicate that all of these extension methods throw a NotSupportedException outside of EF Core.
|
I rebased the PR to the current dev and updated some NuGet settings to target the latest Npgsql build. However, an exception is thrown if I attempt to construct test entities to add and save into the test context... If I comment out the local construct/add/save code, then the unit tests go back to passing. So the CLR to Postgres translation seems to be working fine. It looks like the stack trace is complaining that tuples lack an Do you have any insight into what might be causing this, @roji? |
austindrenski
force-pushed
the
network-address-operators
branch
4 times, most recently
from
eb49120
to
988b768
Compare
Updatesb02bff3 completes the operators for
|
austindrenski
force-pushed
the
network-address-operators
branch
from
126c4b0
to
e8ac7cf
Compare
…xtensions to match NpgsqlDbFunctionsExtensions. Changed network operation extension methods to use DbFunctions instead of IPAddress/NpgsqlInet per the discussion in npgsql#407. Added XML docs to indicate that all of these extension methods throw a NotSupportedException outside of EF Core.
austindrenski
force-pushed
the
network-address-operators
branch
from
472c031
to
4f08f59
Compare
…xtensions to match NpgsqlDbFunctionsExtensions. Changed network operation extension methods to use DbFunctions instead of IPAddress/NpgsqlInet per the discussion in npgsql#407. Added XML docs to indicate that all of these extension methods throw a NotSupportedException outside of EF Core.
- Split out from npgsql#407
austindrenski
changed the title
- Fix macaddr8 mapping - Fix cidr mapping to (IPAddress, int) - Split out from #407
austindrenski
force-pushed
the
network-address-operators
branch
2 times, most recently
from
36e4961
to
748cea8
Compare
- Updated namespaces in extensions to match style. - Changed network extensions to use DbFunctions per npgsql#407. - Added XML docs to indicate these extensionss throw outside of EF Core.
austindrenski
force-pushed
the
network-address-operators
branch
from
748cea8
to
fd05963
Compare
- Updated namespaces in extensions to match style. - Changed network extensions to use DbFunctions per npgsql#407. - Added XML docs to indicate these extensionss throw outside of EF Core.
- cidr - inet - macaddr - macaddr8
- Also some weird SQL being generated.
- Updated namespaces in extensions to match style. - Changed network extensions to use DbFunctions per npgsql#407. - Added XML docs to indicate these extensionss throw outside of EF Core.
- Updates: - Completed NpgsqlNetworkAddressTranslator switch. - Completed NpgsqlNetworkAddressExtensions for inet-inet and cidr-cidr. - Added ClientEvaluationNotSupportedException to throw from provider-specific extension methods. - TODO: - Add non-operators for inet and cidr (i.e. functions). - Problems: - Adding entities that have a tuple throws an exception related to a binary equality operator?
- Maps cidr and inet functions to CLR extension methods. - See: https://www.postgresql.org/docs/current/static/functions-net.html#CIDR-INET-FUNCTIONS-TABLE - Includes functional tests covering translation. - Includes the bug fix in npgsql#425. - This can be removed once npgsql#425 is merged.
- Maps macaddr and macaddr8 functions to CLR extension methods. - macaddr: https://www.postgresql.org/docs/current/static/functions-net.html#MACADDR-FUNCTIONS-TABLE - macaddr8: https://www.postgresql.org/docs/current/static/functions-net.html#MACADDR8-FUNCTIONS-TABLE - Includes functional tests covering translation. - TODO: - Map the standard relational operators and bitwise arithmetic operators.
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
- Includes relational and bitwise operators for `macaddr` and `macaddr8`.
- Mapped to `PhysicalAddress`.
- Includes functional tests covering new operators.
- TODO:
- Add some basic info to the docs.
- BUG:
- Tests failing for `macaddr8`:
```
System.FormatException : MAC addresses must have length 6 in PostgreSQL
at Npgsql.TypeHandlers
.NetworkHandlers
.MacaddrHandler
.ValidateAndGetLength(
PhysicalAddress value,
NpgsqlParameter parameter)
```
- Updates tests for new parentheses in custom binary operators.
- Renames `NetworkAddress*` to `Network*` to reflect the feature scope.
- Issues:
- Tests for `macaddr8` are tricky.
- Entity properties with column type `macaddr8` work fine.
- But parameters of type `PhysicalAddress` are treated as a `macaddr`.
- `System.FormatException : MAC addresses must have length 6 in PostgreSQL`
- Perhaps Npgsql could detect and upgrade `PhysicalAddress` values to `macaddr8`?
- But only when they have 8 bytes?
- Opened issue 428 to discuss possible solutions.
- Adds missing tests for bitwise operators on `PhysicalAddress`. - For `macaddr` and `macaddr8`. - Adds overloads mixing `inet` and `cidr` and related tests.
- ClientEvaluationNotSupportedException.cs - NpgsqlRangeExtensions.cs
austindrenski
force-pushed
the
network-address-operators
branch
from
fd05963
to
3ca6977
Compare
| /// This method is only intended for use via SQL translation as part of an EF Core LINQ query. | ||
| /// </exception> | ||
| public static bool LessThan([CanBeNull] this DbFunctions _, IPAddress inet, IPAddress other) | ||
| => throw ClientEvaluationNotSupportedException(); |
There was a problem hiding this comment.
As discussed elsewhere, let's throw the standard NotSupportedException rather than this.
There was a problem hiding this comment.
This is actually just a helper method now:
static NotSupportedException ClientEvaluationNotSupportedException(
[CallerMemberName] string method = default)
=> new NotSupportedException($"{method} is only intended for use via SQL translation as part of an EF Core LINQ query.");| /// <exception cref="NotSupportedException"> | ||
| /// This method is only intended for use via SQL translation as part of an EF Core LINQ query. | ||
| /// </exception> | ||
| public static bool Equal([CanBeNull] this DbFunctions _, IPAddress inet, IPAddress other) |
There was a problem hiding this comment.
Thinking about this again, this isn't necessary... EF Core already translates x.Equals(y) to x = y in SQL.
So it should be fine to remove all Equal and NotEqual overloads below.
| case nameof(NpgsqlNetworkExtensions.LessThanOrEqual): | ||
| return new CustomBinaryExpression(expression.Arguments[1], expression.Arguments[2], "<=", typeof(bool)); | ||
|
|
||
| case nameof(NpgsqlNetworkExtensions.Equal): |
| case nameof(NpgsqlNetworkExtensions.GreaterThan): | ||
| return new CustomBinaryExpression(expression.Arguments[1], expression.Arguments[2], ">", typeof(bool)); | ||
|
|
||
| case nameof(NpgsqlNetworkExtensions.NotEqual): |
| /// <summary> | ||
| /// Asserts that the SQL fragment appears in the logs. | ||
| /// </summary> | ||
| /// <param name="sql">The SQL statement or fragment |
| /// A <see cref="NetContext"/> for testing. | ||
| /// </returns> | ||
| public NetContext CreateContext() | ||
| { |
There was a problem hiding this comment.
nit: expression-bodied method here and elsewhere
|
Thanks, LGTM. Please squash everything, leaving a single concise commit... |
Map network type operations
See #299 for more information.
<inet '192.168.1.5' < inet '192.168.1.6'<=inet '192.168.1.5' <= inet '192.168.1.5'=inet '192.168.1.5' = inet '192.168.1.5'>=inet '192.168.1.5' >= inet '192.168.1.5'>inet '192.168.1.5' > inet '192.168.1.4'<>inet '192.168.1.5' <> inet '192.168.1.4'<<inet '192.168.1.5' << inet '192.168.1/24'<<=inet '192.168.1/24' <<= inet '192.168.1/24'>>inet '192.168.1/24' >> inet '192.168.1.5'>>=inet '192.168.1/24' >>= inet '192.168.1/24'&&inet '192.168.1/24' && inet '192.168.1.80/28'~~ inet '192.168.1.6'&inet '192.168.1.6' & inet '0.0.0.255'|inet '192.168.1.6' | inet '0.0.0.255'+inet '192.168.1.6' + 25-inet '192.168.1.43' - 36-inet '192.168.1.43' - inet '192.168.1.19'abbrev(inet)abbrev(cidr)broadcast(inet)family(inet)host(inet)hostmask(inet)masklen(inet)netmask(inet)network(inet)set_masklen(inet, int)set_masklen(cidr, int)text(inet)inet_same_family(inet, inet)inet_merge(inet, inet)trunc(macaddr)trunc(macaddr8)macaddr8_set7bit(macaddr8)