protobuf-net-fsharp
protobuf-net-fsharp copied to clipboard
mvkara
Rebase fsharp-collections onto current master and add Set<'t> and Map<'k, 'v>
Upgrading protobuf-net dependency is probably a good idea too: that bug with MetaType.IgnoreListHandling appears to be fixed since. RuntimeTypeModel.IsDefined seems to work somewhat better too, although it still breaks tests if used for Option<'t>
Nice work @mishun. I note that there are conflicts that still need to be resolved. Let me know when this is ready to review. Thanks!
It's rebased onto current master (so it could be merged smoothly into master if necessary). If I understand correctly, when history diverges that much these conflicts can't be resolved automatically, only force-updated.
I tried today, and unfortunately protobuf-net doesn't allow to do anything interesting with existing models for arrays. Looks like ArraySurrogate<'t> has to be removed.
It's rebased onto current master (so it could be merged smoothly into master if necessary)
Understand that. But the way the PR is setup I can't merge it into the master branch. You want to merge it into my POC branch (fsharp-collections) hence Github is complaining. If it is ready for master consumption would rather you point the PR to merge back to master. At this stage, the PR the way that it is, I can't merge it.
Unfortunately, I don't have much experience with github UI (maybe, there's some simple way now), but previously merges like that had to be performed manually: something like clone this repository, add my repository as second remote, fetch fsharp-collections branch from there and force-push it into origin/fsharp-collections. Github was able to understand that pull request is merged.
For master it'll need at least updating protobuf-net dependency to version 3.
Yep I'm ok with that. We just bump the version of the library after significant testing. The Protobuf-net library it is pointing to isn't that new in any case.
If you want this code to eventually go into the library target 'master' and fix any merge conflicts outstanding (so I can review, and if happy hit the Merge button).
What was in CollectionRegistration.fs was moved into ProtobufUtils.fs since after removing ArraySurrogate CollectionRegistration.fs became tiny.
I don't have any prior experience with packet, so I'm not sure I did dependency update correctly
Looks much better. Will review when I get the chance.
Main thing that comes to mind is the Proto representation of the List, Set and other surrogates.
I do have a question that came up in my testing of this code. the answer may be obvious but I'm not seeing it.
Given:
type [<Struct>] TestRecordWithNestedCollections = {
One: string list
Two: Map<int, string list>
Three: string Set
Four: TestUnion
Five: string voption
}
Given the use of surrogates I would expect surrogates to be used. Instead I get the below:
message TestRecordWithNestedCollections {
repeated string One = 1;
repeated KeyValuePair_Int32_ListSurrogate_String Two = 2;
repeated string Three = 3;
TestUnion Four = 4;
FSharpValueOption_String Five = 5;
}
Notice both "One" and "Three" don't use the List or Set surrogates in the generated schema. While I think this is the "correct" behaviour and what I do want I'm trying to work out why this is the case. I can see the 'attemptToRegisterFieldType` called in the 'internalRegister' function which from reading the code should register the surrogates. Feel like I'm missing a detail. Just wondering if the surrogates are actually used here?
I managed to marry both approaches from above (+ zero values handling). Turned out not that bad.
On the topic of zero values: protobuf-net seems to handle empty string ok (it comes out on the other side as empty string, not null). Is manual handling for strings needed?
Notice both "One" and "Three" don't use the List or Set surrogates in the generated schema.
Protobuf-net seems to partially mishandle surrogates for collection types when generating schema (despite that, it do generate "message" for surrogates). That's that suspected bug I wrote above. Surrogates appear to be actually used in serialization/deserialization --- otherwise tests would have failed.
On the topic of zero values: protobuf-net seems to handle empty string ok (it comes out on the other side as empty string, not null). Is manual handling for strings needed?
It used to be to great annoyance. I'm not sure if this has been improved in recent versions - sounds like it maybe has.
Protobuf-net seems to partially mishandle surrogates for collection types when generating schema (despite that, it do generate "message" for surrogates)
Makes sense. Will review in the coming days - apologies for the delay.
At least tests are passing with custom string initialization turned off. There's something weird happening with strings: I found decade old discussion where it's mentioned that strings work ok. Did they broke them later?
Did they broke them later?
Could have. I remember this being an issue when I created this library, and some time before hence I added it as an explicit case. Maybe a Proto V2 problem? It has been quite a long time (years) and things do drift here.
I received an email some time back and have been thinking of how to resolve this, without any obvious answers. Upstream now has FSharp collection serialisers out of the box. See https://github.com/protobuf-net/protobuf-net/tree/main/src/protobuf-net.FSharp
I will engage him to contribute to this discussion.