Network layer and test projects by jackschonherr · Pull Request #12 · HydrologicEngineeringCenter/Consequences

jackschonherr · 2026-08-05T23:18:49Z

Adds NSI API interaction in the Network project and adds a unit testing project which currently has NSI parsing tests (no network tests right now)

Brennan1994

Now that we have a unit test project, we should turn on testing in the CI/CD. Check out the workflows in this repo. I believe they currently pass a run-tests:false flag. flip that guy to true

Brennan1994 · 2026-08-06T23:47:44Z

+/// </summary>
+public static class NsiImporter
+{
+    private const string ROOT = "https://nsi.sec.usace.army.mil/nsiapi/";


Rather than hard code this. Lets use forward link. Check the NSI Importer in the ras-ui project for an example. Forward link lets us control this URL from outside the project, so we can fix old version out in the field when the nsi endpoints change. (hopefully they don't, but the lever is nice to have.)

Brennan1994 · 2026-08-07T00:17:54Z

+    private const string FEATURE_COLLECTION = "&fmt=fc";
+    private const string FEATURE_STREAM = "&fmt=fs";
+
+    private static readonly HttpClient _client = new();


Give this guy a quick peruse. I like the static _client here. Super minor enhancement that follows the microsoft guidelines would be to set up a PooledCollectionLifetime. Other than being nice because it follows the MSdocs, it also means that if we change the fwlink on someone while their process is still running, it'll reset the connection every 15 minutes, instead of waiting till the process exits and boots again. Minor detail, but I'd like to implement. https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines

I'd also like to hold this guy as a default option, but also allow the NsiImporter to have an httpClient injected, so we could give a mock http for unit testing, then we can write tests to verify good behavior with bed error codes from the server, without ever having to touch the network. dotnet does this download file. https://github.com/dotnet/msbuild/blob/e45cc3de8a44d5b92cdad6e0ca7f8a5852c2afbd/src/Tasks.UnitTests/DownloadFile_Tests.cs#L400 -- notice the mock class. the HTTPMEssageHandler lives inside the Client. we can modify that, and throw it in a http client just like they did, and inject this in for testing. Teh actual download file class https://github.com/dotnet/msbuild/blob/main/src/Tasks/DownloadFile.cs

claude suggestion below.

Suggested change

private static readonly HttpClient _client = new();

│ public sealed class NsiImporter │

│ { │

│ // Long-lived shared client. PooledConnectionLifetime bounds DNS staleness so the FWLink │

│ // can actually repoint a running process; Timeout still guards the header phase (F2). │

│ private static readonly HttpClient _shared = new(new SocketsHttpHandler │

│ { │

│ PooledConnectionLifetime = TimeSpan.FromMinutes(15), │

│ }); │

│ │

│ private readonly HttpClient _client; │

│ private readonly Uri _root; │

│ │

│ public NsiImporter(HttpClient? client = null, Uri? root = null) │

│ { │

│ _client = client ?? _shared; │

│ _root = root ?? NsiEndpoints.Default; // the FWLink — see below │

│ } │

│ │

│ /// <summary>Shared importer against the public NSI service.</summary> │

│ public static NsiImporter Default { get; } = new(); │

│ │

│ public Task<List<Building>> ProcessCollectionAsync(string bbox, CancellationToken ct = default) => ... │

│ }

Brennan1994 · 2026-08-07T00:19:30Z

+    {
+        StringBuilder url = new();
+
+        url.Append(ROOT);


This should still work when ROOT changes to the FWLink check that pattern in ras ui

Brennan1994

still reviewing. just submitting so you've got something till I can finsih

…ngineeringCenter/Consequences into network-access-layer

Brennan1994

More comments. Still reviewing.

Brennan1994 · 2026-08-10T21:54:36Z

+        TryMap(structure, out Building building)
+            ? building
+            : throw new KeyNotFoundException(
+                $"NSI structure {structure.FdId} has occupancy type '{structure.Occtype}', " +


Are there other ways this could potentially come out null that isn't teh occupancy type issue?

TryMap will not return null, it returns a bool that will only ever be false when we cannot resolve an occupancy type object from the NSI-provided occtype string. The other building attributes come straight from the NSI

Brennan1994 · 2026-08-10T21:55:28Z

+    /// <see cref="TryMap"/> if you want to filter deliberately.
+    /// </exception>
+    public Building Map(NsiStructure structure) =>
+        TryMap(structure, out Building building)


TryMap is returning a bool. seems odd to not check it for the success state.

We are checking the success state here in the ternary expression, if we resolve a building we return it and if not we throw an exception

Brennan1994 · 2026-08-10T21:57:58Z

+        building = new Building
+        {
+            OccupancyType = occupancyType,
+            Value = (float)structure.ValStruct,


I'm curious how these are handled by the NSI. Do these values always have something associated ot them, or can they ever be unknonw? and if htey are unknown, how are they labeled? do they get 0s, -9999, null? etc. Want to make sure we don't silently swallow an unknown and call it zero. Need to ask Nick how the NSI does this. Will didn't have an answer right away.

I have seen structures with 0 valstruct, not sure if that means unknown. Surely they cannot have truly zero value

Brennan1994 · 2026-08-10T21:58:54Z

+    /// <summary>
+    /// RFC 8142 record separator, prefixed to every feature in an fmt=fs response.
+    /// </summary>
+    public const char RecordSeparator = '\u001e';


What is this?

For the feature stream, every returned feature is prefixed with this character, so this is how we know we are looking at a new feature

Brennan1994 · 2026-08-10T22:21:28Z

+        CancellationToken cancellationToken = default)
+    {
+        string root = await ResolveRoot(cancellationToken);
+        string apiUrl = StructuresEndpoint(root, boundingBox, FEATURE_COLLECTION);


Because of this yeild return, this method quietly returns a state machine, that gives you a structure each time you iterate over it. That's the idea, but a side effect of that is if any of the args are crap. like bounding box being null, or empty, or mapper being null. we won't actually get that exception till someone iterates over the return. Null/whitespace check at the top of this method to avoid that. So we get the exception right away if we have bad args.

Added guards for null or empty. This method is for the feature collection which does not have any yield return, but the stream further along in this file does have that problem. Addressing it there

Brennan1994 · 2026-08-10T22:22:43Z

+
+    public static async IAsyncEnumerable<NsiStructure> ParseFeatureStreamAsync(
+        TextReader reader,
+        [EnumeratorCancellation] CancellationToken cancellationToken = default)


Token attribute here is cool. Had to look it up. nice.

Brennan1994 · 2026-08-10T22:57:58Z

+    /// Downloads the whole feature collection, projecting each structure with
+    /// <paramref name="mapper"/>. Swap the mapper to import a different receptor type.
+    /// </summary>
+    public async Task<List<TReceptor>> ProcessCollection<TReceptor>(


Get stream async throws away the http response object from the server and just gives us the body. That's cool sometimes, but we're losing data on failure. That response object has actionable information in it we should communicate to ourselves and users. We can get around this by breaking the network request into two peices. Read headers, Read Content. Check out the last method here: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/HttpClientJsonExtensions.Get.AsyncEnumerable.cs

Dotnet follows this pattern. Read the headers, Read the stream, Deserialize with a yeild return. This is the same game we're playing.

Brennan1994

final comments in.

Brennan1994 · 2026-08-11T17:58:01Z

+/// be deserialized straight into <see cref="Building"/> — the mapper supplies what the
+/// wire format doesn't.
+/// </summary>
+public sealed class BuildingMapper : INsiStructureMapper<Building>


Can we also implement tests and a mapper for the lifeloss building? I suspect it's going to reveal some deficiencies in the INsiStructureMapper.

I did not do this yet because life loss buildings have a number of other required attributes that I do not know how to resolve such as # able-bodied people, access to attic, etc.

Brennan1994 · 2026-08-11T18:03:17Z

+        using Stream jsonResponse = await _client.GetStreamAsync(apiUrl, cancellationToken);
+
+        List<NsiStructure> structures =
+            await NsiJsonParser.ParseFeatureCollectionAsync(jsonResponse, cancellationToken);


Could potentially simplify this a bit: drop the string builder for a regular interpolated string. $"{ROOT}structures?bbox={Uri.EscapeDataString(boudningBox)&fmt={directive}" That Uri.EscapeDataString should protect against rogue whitespace in the bounding box list. like a space after a comma where there shouldn't be.

Brennan1994 · 2026-08-11T18:36:39Z

+public sealed record NsiGeometry
+{
+    [JsonPropertyName("type")]        public string Type              { get; init; } = "";
+    [JsonPropertyName("coordinates")] public double[] Coordinates     { get; init; } = [];


Record here implies immutability. This is probably minor, as we're not going to be doing a bunch of NsiGeometry a == NsiGeometry b, but I think it'd be good form to try to make that implied immutability true. I think you can just swap double[] for a ReadOnlyMemory

Brennan1994 · 2026-08-11T18:48:46Z

+
+    // PooledConnectionLifetime per the HttpClient guidelines: a client this long-lived would
+    // otherwise hold connections that never notice DNS moving underneath them.
+    // https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines


Can we make the name a little more descriptive? maybe something like GetBuildingsAsync?

Brennan1994 · 2026-08-11T18:50:51Z

+    });
+
+    private readonly HttpClient _client;
+    private readonly string? _fixedRoot;


can we use a different word than projecting for this? maybe unpacking? We just use the term projection so much in our domain, I'd like to keep its potential meanings limited.

Brennan1994 · 2026-08-11T19:05:21Z

+/// The API root is resolved through <see cref="HecFwLink"/> rather than being compiled in,
+/// so NSI can move without a release of this library.
+///
+/// Most callers want <see cref="Default"/>. Construct one to supply an <see cref="HttpClient"/>


As matter of convention, asynchronous methods should be suffixed with the name async, like HttpClient's are, and include in the name what it returns as specific as you can for the method. Give these guys a review and run through and see if you can incorporate that. And that async suffix doesn't only apply to methods with the keyword async, but ones that return tasks. Official documentation of this is here: https://learn.microsoft.com/en-us/dotnet/standard/asynchronous-programming-patterns/task-based-asynchronous-pattern-tap . It's more than I think is worth reading unless you're feeling really dedicated. Might be a nice claude summarizable learning material though.

Brennan1994 · 2026-08-11T19:07:47Z

+{
+    private readonly Dictionary<string, OccupancyType> _occupancyTypes;
+
+    public BuildingMapper(IEnumerable<OccupancyType> occupancyTypes)


Should add a gaurd and exception for duplicate occupancy type names. They'll throw already I think, but you oculd give a more useful message, like which occtype is duplicated

Brennan1994 · 2026-08-11T19:09:08Z

+    <TargetFramework>net9.0</TargetFramework>
+    <ImplicitUsings>enable</ImplicitUsings>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>


Add a packable true here. We'll want to publish this guy as a nuget

Network layer and test projects

24823ed

jackschonherr requested a review from Brennan1994 August 5, 2026 23:18

Brennan1994 reviewed Aug 6, 2026

View reviewed changes

Brennan1994 requested changes Aug 7, 2026

View reviewed changes

jackschonherr added 4 commits August 7, 2026 08:54

Enable test execution in CI workflow

69a1253

fwlink

8730a1e

better http client, allow client injection for testing

2f854d4

Merge branch 'network-access-layer' of https://github.com/HydrologicE…

6c091f8

…ngineeringCenter/Consequences into network-access-layer

Brennan1994 requested changes Aug 10, 2026

View reviewed changes

Brennan1994 requested changes Aug 11, 2026

View reviewed changes

establish Scrach space.

cee3ca2

-    private static readonly HttpClient _client = new();
+│ public sealed class NsiImporter                                                                                     │
+│ {                                                                                                                   │
+│     // Long-lived shared client. PooledConnectionLifetime bounds DNS staleness so the FWLink                        │
+│     // can actually repoint a running process; Timeout still guards the header phase (F2).                          │
+│     private static readonly HttpClient _shared = new(new SocketsHttpHandler                                         │
+│     {                                                                                                               │
+│         PooledConnectionLifetime = TimeSpan.FromMinutes(15),                                                        │
+│     });                                                                                                             │
+│                                                                                                                     │
+│     private readonly HttpClient _client;                                                                            │
+│     private readonly Uri _root;                                                                                     │
+│                                                                                                                     │
+│     public NsiImporter(HttpClient? client = null, Uri? root = null)                                                 │
+│     {                                                                                                               │
+│         _client = client ?? _shared;                                                                                │
+│         _root   = root   ?? NsiEndpoints.Default;   // the FWLink — see below                                       │
+│     }                                                                                                               │
+│                                                                                                                     │
+│     /// <summary>Shared importer against the public NSI service.</summary>                                          │
+│     public static NsiImporter Default { get; } = new();                                                             │
+│                                                                                                                     │
+│     public Task<List<Building>> ProcessCollectionAsync(string bbox, CancellationToken ct = default) => ...          │
+│ }

Uh oh!

Conversation

jackschonherr commented Aug 5, 2026

Uh oh!

Brennan1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brennan1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Brennan1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brennan1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants