Skip to content

Add IPNetwork.Parse and TryParse#44573

Merged
Tratcher merged 6 commits intodotnet:mainfrom
david-acker:add-ipnetwork-parse
Oct 18, 2022
Merged

Add IPNetwork.Parse and TryParse#44573
Tratcher merged 6 commits intodotnet:mainfrom
david-acker:add-ipnetwork-parse

Conversation

@david-acker
Copy link
Member

@david-acker david-acker commented Oct 15, 2022 •
edited
Loading

Add Parse and TryParse methods for IPNetwork

Adds Parse and TryParse methods for creating an IPNetwork using an input string in CIDR notation or 'slash notation'

Description

Adds the following Parse and TryParse static methods to IPNetwork:

public static IPNetwork Parse(ReadOnlySpan<char> networkSpan);
public static bool TryParse(ReadOnlySpan<char> networkSpan, out IPNetwork? network);

Examples

IPv4

// Using Parse
var network = IPNetwork.Parse("192.168.0.1/32");

// Using TryParse
bool success = IPNetwork.TryParse("192.168.0.1/32", out var network);

// Ctor equivalent
var network = new IPNetwork(IPAddress.Parse("192.168.0.1"), 32);

IPv6

// Using Parse
var network = IPNetwork.Parse("2001:db8:3c4d::1/128");

// Using TryParse
bool success = IPNetwork.TryParse("2001:db8:3c4d::1/128", out var network);

// Ctor equivalent
var network = new IPNetwork(IPAddress.Parse("2001:db8:3c4d::1"), 128);

Fixes #8606

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Oct 15, 2022
@ghost
Copy link

ghost commented Oct 15, 2022

Thanks for your PR, @david-acker. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

gfoidl
gfoidl reviewed Oct 16, 2022
View reviewed changes
Copy link
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits / hints.

src/Middleware/HttpOverrides/src/IPNetwork.cs Outdated
return false;
}

if (prefix.AddressFamily == AddressFamily.InterNetworkV6 && prefixLength > 128)
Copy link
Member

@gfoidl gfoidl Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrleated to your change: but these should be if-else if-else to avoid double-checks, or actually I'd write it as

static bool IsValidPrefixLengthRange1(IPAddress prefix, int prefixLength)
{
    return prefix.AddressFamily switch
    {
        AddressFamily.InterNetwork => prefixLength <= 32,
        AddressFamily.InterNetworkV6 => prefixLength <= 128,
        _ => prefixLength >= 0
    };
}

and let Roslyn and JIT optimize it.

Copy link
Member Author

@david-acker david-acker Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good point. Although with your example, the prefixLength >= 0 check isn't be enforced for the non-default cases so negative prefixLength values would slip through. How about this instead?

static bool IsValidPrefixLengthRange(IPAddress prefix, int prefixLength)
{
+   if (prefixLength < 0)
+   {
+       return false;
+   }

    return prefix.AddressFamily switch
    {
        AddressFamily.InterNetwork => prefixLength <= 32,
        AddressFamily.InterNetworkV6 => prefixLength <= 128,
-       _ => prefixLength >= 0
+      _ => true
    };
}

Copy link
Member

@gfoidl gfoidl Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right 👍🏻

@danmoseley
Copy link
Member

danmoseley commented Oct 16, 2022

@MihaZupan this type seems like something you'd expect in System.Net. I wonder whether there is missing API there.

@MihaZupan
Copy link
Member

MihaZupan commented Oct 16, 2022

We've had a few issues about CIDR support in System.Net: dotnet/runtime#36428, dotnet/runtime#64033, dotnet/runtime#42845
So far they've all been rejected/gone quiet due to lack of demand/being a bit niche.

It looks like users are satisfied with the NuGet package that offers all sorts of these helpers https://github.com/jsakamoto/ipaddressrange and haven't been pushing on the runtime team.

@build-analysis build-analysis bot mentioned this pull request Oct 16, 2022
Closed
2 tasks
@slang25
Copy link
Contributor

slang25 commented Oct 16, 2022

There's no package out there that I'm completely happy with. I'd like something in System.Net that has good performance (a struct implementation would be ideal).

I rolled my own version of this last week to avoid adding a dependency to my library.

@danmoseley
Copy link
Member

danmoseley commented Oct 17, 2022 •
edited
Loading

I'd like something in System.Net that has good performance (a struct implementation would be ideal).

If you want to pursue that, you'll want to create an API proposal there of course. Edit: oh, I misread, sounds like we already have discussion there.

@adityamandaleeka adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label Oct 17, 2022
@ghost
Copy link

ghost commented Oct 17, 2022

@david-acker, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

@Tratcher Tratcher merged commit 312706f into dotnet:main Oct 18, 2022
@Tratcher
Copy link
Member

Tratcher commented Oct 18, 2022

Thanks

@ghost ghost added this to the 8.0-preview1 milestone Oct 18, 2022
@david-acker david-acker deleted the add-ipnetwork-parse branch October 21, 2022 15:13
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

+1 more reviewer

@gfoidl gfoidl gfoidl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@adityamandaleeka adityamandaleeka

Labels

area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares blog-candidate Consider mentioning this in the release blog post community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0-preview1

Development

Successfully merging this pull request may close these issues.

Consider adding IPNetwork.Parse

8 participants

@david-acker @danmoseley @MihaZupan @slang25 @Tratcher @gfoidl @adityamandaleeka @amcasey