Skip to content

[Repo Assist] refactor: extract fetchUrlContent helper to eliminate HTTP fetch duplication in Utils.fs#406

Merged
sergey-tihon merged 6 commits into
masterfrom
repo-assist/improve-utils-http-refactor-20260425-29d415d0bbe57ad6
Apr 25, 2026
Merged

[Repo Assist] refactor: extract fetchUrlContent helper to eliminate HTTP fetch duplication in Utils.fs#406
sergey-tihon merged 6 commits into
masterfrom
repo-assist/improve-utils-http-refactor-20260425-29d415d0bbe57ad6

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant.

Summary

Extracts a private fetchUrlContent helper from readSchemaPath in Utils.fs, eliminating ~60 lines of code duplication between the "https" and "http" URL scheme branches.

Before

Both branches independently built the same header dictionary, created the same HttpRequestMessage/HttpClient, sent the request, and handled errors — ~50 lines duplicated verbatim.

After

A single fetchUrlContent helper handles the shared logic. Each scheme branch is reduced to its unique part:

| "https" ->
    validateSchemaUrl ignoreSsrfProtection uri
    return! fetchUrlContent ignoreSsrfProtection headersStr resolvedPath

| "http" ->
    if not ignoreSsrfProtection then
        return failwithf "HTTP URLs are not supported..."
    else
        validateSchemaUrl ignoreSsrfProtection uri
        return! fetchUrlContent ignoreSsrfProtection headersStr resolvedPath

Minor fix included

The original HTTP branch did not handle Swagger.OpenApiException (it would bubble up unhandled). The shared helper now handles it consistently for both HTTP and HTTPS, matching the original HTTPS behaviour.

Test Status

All 327 unit tests pass (same as baseline, no regressions).
Fantomas format check passes (dotnet fantomas --check src/SwaggerProvider.DesignTime/Utils.fs).

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@3de4e604a36b5190a1c7dc4719c7341500ba8a95

…nate duplication

The HTTPS and HTTP branches of readSchemaPath shared ~50 lines of
identical code for building headers, creating the HttpRequestMessage,
creating the HttpClient, sending the request, and handling errors.

Extract a private fetchUrlContent helper that:
- Parses custom headers from the pipe-separated header string
- Creates and sends the GET request with UseDefaultCredentials=false
- Validates the response Content-Type
- Handles all three error cases uniformly (OpenApiException, WebException, other)

The HTTP branch now also handles OpenApiException (previously it would
bubble up unhandled), which is a minor bug fix alongside the cleanup.

readSchemaPath is reduced by ~60 lines with identical behaviour preserved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sergey-tihon sergey-tihon marked this pull request as ready for review April 25, 2026 15:32
Copilot AI review requested due to automatic review settings April 25, 2026 15:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors SchemaReader.readSchemaPath to remove duplicated HTTP fetch logic by extracting a shared private helper, keeping the SSRF-guarded URL scheme branching intact while centralizing request/response handling.

Changes:

  • Added a private fetchUrlContent helper to consolidate GET + headers + content-type validation + error handling.
  • Replaced duplicated "https" and "http" fetch implementations with calls to the shared helper.
  • Standardized exception handling behavior across both schemes (notably Swagger.OpenApiException vs WebException handling paths).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/SwaggerProvider.DesignTime/Utils.fs Outdated
Comment thread src/SwaggerProvider.DesignTime/Utils.fs Outdated
Comment thread src/SwaggerProvider.DesignTime/Utils.fs Outdated
Comment thread src/SwaggerProvider.DesignTime/Utils.fs Outdated
@sergey-tihon
Copy link
Copy Markdown
Member

@copilot fix inline comments and check that there is no breaking changes.

Copilot AI and others added 2 commits April 25, 2026 17:35
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 25, 2026

@copilot fix inline comments and check that there is no breaking changes.

Fixed all inline comments in commits f43d9c8 and e0959ae:

  1. Header parsing (Split '='Split([|'='|], 2) + .Trim()): headers with = in the value (e.g., Base64-encoded Authorization) are now handled correctly.
  2. Disposal: HttpRequestMessage uses use request = ... and HttpResponseMessage uses use! response = ... to ensure both are disposed even on exceptions.
  3. No Async.RunSynchronously: the OpenApiException branch now uses let! content = ex.Content.ReadAsStringAsync() |> Async.AwaitTask directly in the outer async block's match arm.
  4. Stack trace preservation: replaced failwith(e.ToString()) with e.Reraise().
  5. Removed the unnecessary nested async { } wrapper around the match expression — let!/return are used directly in the outer async block.

All 327 unit tests pass and Fantomas format check passes.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/SwaggerProvider.DesignTime/Utils.fs Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@sergey-tihon
Copy link
Copy Markdown
Member

@copilot fix build and formatting

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 25, 2026

@copilot fix build and formatting

Fixed in commit 4ab7820 — applied Fantomas formatting to the HttpClientHandler constructor call that was split across two lines after the AllowAutoRedirect change. All 327 unit tests pass and format check is clean.

@sergey-tihon sergey-tihon merged commit 735814c into master Apr 25, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants