Skip to content

[Repo Assist] fix: use toParam in form content helpers for correct date/time serialization (+4 tests, 296→300)#395

Open
github-actions[bot] wants to merge 6 commits intomasterfrom
repo-assist/improve-form-content-date-formatting-20260421-c5f8cfaceb190ace
Open

[Repo Assist] fix: use toParam in form content helpers for correct date/time serialization (+4 tests, 296→300)#395
github-actions[bot] wants to merge 6 commits intomasterfrom
repo-assist/improve-form-content-date-formatting-20260421-c5f8cfaceb190ace

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

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

Summary

toFormUrlEncodedContent and toMultipartFormDataContent previously called x.ToString() / v.ToString() on field values, which gives locale-dependent output for DateTime/DateTimeOffset and incorrect output for DateOnly. They now delegate to toParam, which applies the same ISO 8601 formatting already used for query parameters.

Root cause

Both helpers were written before toParam existed as a dedicated serialization function. The TODO comment on the old code (// TODO: serialize? does not work with arrays probably) flagged the gap.

Changes

  • RuntimeHelpers.fs:
    • toMultipartFormDataContent: replace x.ToString() with toParam x (skips null result — handles F# None options)
    • toFormUrlEncodedContent: replace Seq.map ... v.ToString() with Seq.choose ... toParam v (excludes entries where toParam returns null)
  • RuntimeHelpersTests.fs: 4 new tests verifying ISO 8601 formatting of DateTime and DateTimeOffset values in both form content types

Test Status

dotnet build — succeeded (0 errors)
✅ Unit tests — 300 passed, 0 failed (1 skipped pre-existing)
dotnet fantomas --check — no formatting issues

Trade-offs

  • toParam returns null for None options; the updated code skips those entries rather than serializing "". This is consistent with how getPropertyValues already filters out None values before calling these helpers.
  • Arrays are still not expanded into multiple form entries; that remains a known limitation (the TODO comment has been removed since the fix is now partially addressed via toParam delegation).

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

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@96b9d4c39aa22359c0b38265927eadb31dcf4e2a

…ntent for correct date/time serialization (+4 tests, 296→300)

Previously both functions used ToString() which gives locale-dependent output
for DateTime/DateTimeOffset and incorrect formatting for DateOnly values.
They now delegate to toParam, which uses ISO 8601 (O specifier) for DateTime
and DateTimeOffset, and yyyy-MM-dd for DateOnly — the same formats used
for query parameters.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sergey-tihon sergey-tihon marked this pull request as ready for review April 22, 2026 07:32
Copilot AI review requested due to automatic review settings April 22, 2026 07: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

Updates form-content serialization to use the existing toParam helper so DateTime/DateTimeOffset/DateOnly values are formatted consistently (ISO 8601 / round-trip) instead of relying on locale-dependent ToString() output.

Changes:

  • toMultipartFormDataContent: serialize non-stream values via toParam, skipping values where toParam returns null (e.g., None)
  • toFormUrlEncodedContent: serialize values via toParam, excluding entries where toParam returns null
  • Add 4 unit tests covering DateTime/DateTimeOffset formatting in both form content types

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/SwaggerProvider.Runtime/RuntimeHelpers.fs Routes form body field serialization through toParam to ensure consistent ISO formatting and option handling.
tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs Adds regression tests for DateTime/DateTimeOffset formatting for both url-encoded and multipart form bodies.

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

Comment thread tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs
Comment thread tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs Outdated
Comment thread tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs
Comment thread tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs Outdated
@sergey-tihon
Copy link
Copy Markdown
Member

@copilot review inline comments and address what is applicable

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 22, 2026

@copilot review inline comments and address what is applicable

Addressed the applicable inline review feedback in 77603d7 by tightening the new DateTime/DateTimeOffset tests to assert exact round-trip values (ToString("O")) after URL decoding for form-url-encoded content and exact body equality for multipart content. Targeted RuntimeHelpers test modules pass locally. No UI changes in this PR.

@sergey-tihon
Copy link
Copy Markdown
Member

@copilot should data in form content be url encoded? it is expected behavior?

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 22, 2026

@copilot should data in form content be url encoded? it is expected behavior?

Yes—application/x-www-form-urlencoded data is expected to be URL-encoded, and toFormUrlEncodedContent does that via FormUrlEncodedContent. For multipart/form-data, values are sent as multipart parts (not URL-encoded query-style), which is also expected behavior. No UI changes in this PR, so no screenshot is applicable.

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 2 out of 2 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 tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 22, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • petstore.swagger.io
    • Triggering command: /usr/share/dotnet/dotnet /usr/share/dotnet/dotnet /usr/share/dotnet/sdk/10.0.201/FSharp/fsc.dll @/tmp/MSBuildTempgHY5nu/tmp52e9d649019d473999a70181bd39f467.rsp (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

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 2 out of 2 changed files in this pull request and generated no new comments.


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

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