Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Linq support for FSharp options #3589

Merged
merged 19 commits into from
Dec 12, 2024
Merged

Conversation

nkosi23
Copy link

@nkosi23 nkosi23 commented Dec 10, 2024

Summary

This PR adds support for querying against F# options (the F# native equivalent to C#'s nullables).
Because F# options are a generic concrete type and not an interface, there is no covariance. This has required introducing a number of adjustments:

IValueTypeMember is now generic

public interface IValueTypeMember<TOuter, TInner>: IQueryableMember
{
    IEnumerable<TInner> ConvertFromWrapperArray(IEnumerable<TOuter> values);
    ISelectClause BuildSelectClause(string fromObject);
}

This was mainly required because the boxing introduced by ConvertFromWrapperArray caused casting exceptions for F# options since covariance is not possible. Most of the changes are just small API modifications required on components consuming this interface.

Unit tests

A number of unit tests have been created. One noteworthy fact is that comparing F# options (>, >=, <, <=) is invalid in C# (compiler syntax error) while it is a valid construct in F#.

I have therefore defined the comparison lambda expressions in the F# library of the test suite. This required moving the Target test type to a standalone library since the F# lambdas need to reference it (otherwise there'd be a circular dependency since the main test project needs to reference the F# lambdas).

Nullable struct value types

Nullable struct value types required special treatment (eg. IssueId?) to avoid casting issues since IValueType is now strongly typed. Before calling ConvertFromWrapperArray(values) there was a need to unwrap the values from their nullable.

This has resulted in the initially daunting UnwrapIEnumerableOfNullables(this object obj) method which is actually simple to understand when we take a closer look (we check if it's a IEnumerable<Nullable>, if yes we unwrap and create an instance of the strongTyped id either through the constructor or through the builder method).

Some F# tests are kept separate from other tests

Serializing F# options properly requires using System.Text.Json (at least this is the use case I was targetting, since it is the most commonly used serializer nowadays. The most popular f# serialization library uses System.Text.Json. And this is what I use). Using System.Text.Json globally was breaking a couple of tests (this was mainly due to the fact that STJ struggles to deserialize anonymous types). I have therefore created separate F#-friendly stores and created a separate test class using these stores.

Remarks

The implementation assumes that options are stored in the DB unwrapped. While in my opinion this is what makes the most sense (there is either the json property or there is nothing), this sort of puts a constraint on the serialization format (serializers can serialize options in plenty of ways since F# options are discriminated unions).

In a later version, we may look into enabling the user to customize the serialization format somehow (without special handling, just supplying an IMemberSource may not be enough since the built-in FSharpOptionQueryableMember is already registered), but i think this is very reasonable for a default and a starting point

Also in terms of use, users will have to register all the option types variations themselves on startup:

opts.RegisterValueType(typeof(FSharpOption<Guid>));
opts.RegisterValueType(typeof(FSharpOption<string>));
opts.RegisterValueType(typeof(FSharpOption<long>));
...

This is not ideal, especially since these types are well-known, but I couldn't find a place where value types are auto-registered / built-in. Maybe we can create an extension method on StoreOptions RegisterFsharpOptionsValueTypes()?

@nkosi23
Copy link
Author

nkosi23 commented Dec 10, 2024

i have just added a RegisterFsharpOptionsValueTypes() + additional tests

@nkosi23
Copy link
Author

nkosi23 commented Dec 10, 2024

Please do not merge yet, I need to fix failing regression tests (there is a single root cause). Nullable structs are not properly handled (eg. Issue2d?)

@nkosi23
Copy link
Author

nkosi23 commented Dec 11, 2024

The problem should now be fixed, tests are passing

nkosi23 added 12 commits December 11, 2024 01:44
- Required putting the Target type in a separate shared library to avoid a circular dependency
- Moved Fsharp acceptance tests to different test classes to avoid affecting other tests due to the specific serializer used for F#
Improved separation between F# test cases and C# test cases to avoid breaking other tests due to differences with the serializer used (newtonsoft has limited support for discriminated unions).
@nkosi23
Copy link
Author

nkosi23 commented Dec 11, 2024

@jeremydmiller Hi Jeremy ! I know I've opened this PR literally just yesterday but I really hope this can make it through the next release as I've implemented this on borrowed time 😅 I was already behind schedule for my project supposed to be used by the customer during the holidays season...

@jeremydmiller jeremydmiller merged commit 159781d into JasperFx:master Dec 12, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants