Repository navigation
feat(json): add a converter of F# records that lets fields be absent - #77
Open
xperiandri wants to merge 2 commits into
Open
xperiandri wants to merge 2 commits into
xperiandri wants to merge 2 commits into
Conversation
…osmos.Json` The converters do not use the Azure Cosmos DB SDK, but the package of the library for the API for NoSQL depends on the SDK. Thus a project that uses the converters from that package also depends on the SDK. A client of the API for Gremlin can use `TimeSpanTicksConverter`, but it does not use the SDK. The new project `src/Cosmos.Json` makes the package `FSharp.Azure.Cosmos.Json`. It contains `JsonConverters.fs` without a change and depends only on `FSharp.Core`. `FSharp.Azure.Cosmos` references the new project. Thus its package depends on `FSharp.Azure.Cosmos.Json` of the same version. The namespace of the converter does not change. Thus the code that uses the converter does not change. No released package contains `TimeSpanTicksConverter`. Thus a type forwarder is not necessary. The tests of the converter are in `tests/Cosmos.Tests` as before. They use the new assembly through the reference to `FSharp.Azure.Cosmos`, and the emulator tests of the converter use the client of that library. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 28 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved converter defects produce incorrect struct defaults, reject permitted null value options, and fail serialization or deserialization when ignored and active fields share a JSON name.
3 open issues
What changed in this PR
Adds configurable F# record converters that support absent and null fields in Cosmos DB documents. The PR also includes the JSON package extraction from #73, allowing converters to be used without the Cosmos DB SDK.
Changes:
- Adds record conversion with field omission, naming aliases, ignore conditions, and strict or permissive reading.
- Adds unit tests covering serialization, deserialization, and compatibility behavior.
- Integrates the JSON package and documents its setup and dependencies.
| File | Description |
|---|---|
tests/Cosmos.Tests/TestCategories.fs |
Adds the record converter test category. |
tests/Cosmos.Tests/Json/AllowNullFSharpRecordConverterTests.fs |
Tests record conversion and edge cases. |
tests/Cosmos.Tests/FSharp.Azure.Cosmos.Tests.fsproj |
Includes the new tests. |
src/Cosmos/FSharp.Azure.Cosmos.fsproj |
References the extracted JSON project. |
src/Cosmos.Json/RecordConverter.fs |
Implements the record converter and factory. |
src/Cosmos.Json/JsonConverters.fs |
Relocates the existing duration converter unchanged. |
src/Cosmos.Json/FSharp.Azure.Cosmos.Json.fsproj |
Defines the JSON package and dependencies. |
src/Cosmos.Json/AssemblyInfo.fs |
Supplies JSON assembly metadata. |
README.md |
Lists the JSON package. |
FSharp.Azure.Cosmos.slnx |
Adds the JSON project to the solution. |
FSharp.Azure.Cosmos.slnf |
Includes the JSON project in the solution filter. |
docsSrc/How_Tos/Store_Records_With_Absent_Fields.md |
Documents record converter setup and behavior. |
docsSrc/How_Tos/Store_Durations.md |
Explains the duration converter’s package location. |
Directory.Packages.props |
Pins the FSharp.SystemTextJson dependency. |
CHANGELOG.md |
Records the package extraction and record converters. |
.github/instructions/test-layout.instructions.md |
Places record converter tests in the JSON group. |
.github/copilot-instructions.md |
Updates the documented solution structure. |
Review effort: Balanced
The Ecierge application has a converter of F# records of its own: a document can leave out a field or have null for it, and the converter does not write a field without a value. FSharp.SystemTextJson writes each field of a record. The converter moves into the package FSharp.Azure.Cosmos.Json as `AllowNullFSharpRecordConverterFactory` and `AllowNullFSharpRecordConverter<'Record>`, with these corrections: - It writes a field under the name of `JsonName` or `JsonPropertyName`, as it reads it. The converter of Ecierge wrote the name of the naming policy, so a record with `[<JsonPropertyName "id">]` had no `id` in Cosmos DB. - It leaves out `ValueNone`, as it leaves out `None` and `Skip`. - It gives an ignored field of a value type its default value. FSharp.SystemTextJson 1.4.36 throws NullReferenceException there (issue 217 of FSharp.SystemTextJson). The default value of a struct has all bits zero: the converter does not call a constructor of the struct. - It skips an unknown property with `TrySkip`. `Skip` throws when the serializer reads a stream in parts, for example a page of a query. - It does not write null for a value that the serializer cannot write, and it has no `Debugger.Break`. - It reads null for a field of a record type. For a field whose converter refuses null, it gives the value without a value. - When null fields are not allowed, it reads null for an option and refuses a document without a field that is not an option. An option that the serializer cannot read from null, for example a voption without UnwrapOption, gets its value without a value. - Two fields that it writes, or two fields that it reads, cannot have one name. A field with JsonIgnore keeps no name, so a new field can take the name of an old field. - It obeys `WhenWriting`, `WhenReading` and `UnmappedMemberHandling`, and it takes the records of a module and the records with a private representation. A function chooses the records, not a namespace. FSharp.Azure.Cosmos.Json now depends on FSharp.SystemTextJson, for `JsonNameAttribute` and `Skippable`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
xperiandri
force-pushed
the
feat/json-record-converter
branch
from
November 8, 2026 03:32
e2eef03 to
1118092
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Active issues break the Include None round trip by dropping the property and incorrectly reject distinct write-side property names when case-insensitive reading is enabled.
2 open issues
3 resolved since last review
Review effort: Balanced
Comment on lines
+191
to
+196
| if reader.TokenType <> JsonTokenType.Null then | ||
| deserialize &reader options | ||
| // FSharp.SystemTextJson cannot read null as a Skippable of a value type | ||
| elif isSkippable then | ||
| absentValue | ||
| elif not (isOptional || allowNullFields) then |
| else | ||
| StringComparer.Ordinal | ||
|
|
||
| let written = Dictionary<string, int> comparer |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Ecierge application has a converter of F# records of its own: a document can leave out a field or have
nullfor it, and the converter does not write a field without a value. FSharp.SystemTextJson writes each field of a record. This change moves that converter intoFSharp.Azure.Cosmos.JsonasAllowNullFSharpRecordConverterFactoryandAllowNullFSharpRecordConverter<'Record>, and corrects its errors. The how-to pageStore_Records_With_Absent_Fields.mdshows the set-up and the rules.This pull request contains the commit of #73 (
8a8d379), which adds the package. CI runs only for pull requests intomain, so the base ismain. After #73 merges, I rebase this branch.What is corrected against the converter of Ecierge
ModelCleanupContextwith[<JsonPropertyName "id">] ModelIdhad noid.ValueNoneasnulland left outNone.reader.Skip ()threw for a record inside an array that the serializer read from a stream in parts, for example a page of a query (checked in F# Interactive).reader.TrySkip ().nullwhen the serializer could not write a value.Debugger.Break ()forWhenWritingandWhenReadingofJsonIgnore.nullin a field of a record type or of a value type.nullor its default value.nullfor an option and accepted a document without a field.nullfor an option and refuses a document without a field that is not an option.JsonIgnore.Ecierge, not the records of a module or with a private representation.FSharp.SystemTextJson 1.4.36 throws
NullReferenceExceptionwhen it reads a record with an ignored field of a value type (Tarmil/FSharp.SystemTextJson#217). This converter gives such a field its default value.Also, the default value of a struct has all bits zero, without a call of a constructor of the struct. A
voptionthat the serializer cannot read fromnull(noUnwrapOption) getsValueNone. Two fields that the converter writes, or two fields that it reads, cannot have one name, but a new field can take the name of a field withJsonIgnore.A new dependency
FSharp.Azure.Cosmos.Json, and thusFSharp.Azure.Cosmos, now depends onFSharp.SystemTextJson, forJsonNameAttributeandSkippable. #65 adds the same line toDirectory.Packages.props, so the second of the two to merge gets a small conflict.For Ecierge
Replace
Ecierge.Json.AllowNullFSharpRecordConverterFactory (true)withFSharp.Azure.Cosmos.AllowNullFSharpRecordConverterFactory (true, fun recordType -> ...)and give the function the condition of the oldCanConvert. A field withJsonPropertyNameorJsonNameis then written under that name. The converter still reads the documents that have the name of the naming policy.Tests
tests/Cosmos.Tests/Json/AllowNullFSharpRecordConverterTests.fs: 26 tests, categoryRecordConverterin the groupJson/. Each row of the table has a test.tests/Cosmos.Testspass. No new warning. Fantomas accepts the files.Not in this change
The narrow layout (#65) and the Gremlin graph model describe only the records that FSharp.SystemTextJson writes. They can describe the records of this converter too, in a change of its own.
🤖 Generated with Claude Code