fsprojects / FSharp.Json

F# JSON Reflection based serialization library
Apache License 2.0
222 stars 26 forks source link

Can not serialize/deserialize type with "internal" or "private" modifier #33

Open aleksandrkolesnikov opened 4 years ago

aleksandrkolesnikov commented 4 years ago

Hello. When I try to serialize/deserialize Person, I have an exception

FSharp.Json.JsonSerializationError: 'Failed to serialize, must be one of following types: record, map, array, list, tuple, union. Type is: Person.'

But if I remove internal modifier for Person, the code works fine



type internal Person = {

    [<JsonField("name")>]
    Name: string

    [<JsonField("age")>]
    Age: int
}

[<EntryPoint>]
let main argv =
    let person = { Name = "John"; Age = 42 }
    let json = Json.serialize person
    0
sanchez commented 4 years ago

I believe from a dotnet perspective, it's not possible to view type information of a type that is not made public. So it's not possible to view the properties on the object

vsapronov commented 4 years ago

@aleksandrkolesnikov What @sanchez wrote is true. Do you have some suggestion on how to retrieve type information for internal or private types?

bzuu-ic commented 3 years ago

This can be done by using the correct BindingFlags. See this commit for an example. I'd be happy to help get this implemented.

bartelink commented 3 years ago

While this should not be the primary consideration, private reflection is a lot less efficient. That's one reason why Newtonsoft has the same 'weakness'. (Pretty sure STJ does too). If this was a perf-oriented impl, one might at a minimum expect this to be an opt-in feature at a minimum...

bzuu-ic commented 3 years ago

@bartelink I wasn't aware of the performance implications, but even if the performance would be equal I think this should not be the default behaviour indeed. An overload that allows this would be quite welcome though.

bartelink commented 3 years ago

I think this should not be the default behaviour indeed

I get that; I've been bitten by it too, and it is can be very frustrating, but the fact that this would place this lib on its own in terms of opting people into the cost of private reflection.

Flipping a default like this can be considered a breaking change, depending on how you look at it.

(Its also possible that there's been perf optimization work done to special case the loading such that you only pay extra cost when they are private - that would probably be something that people would be less surprised by, if it were the case...)

bzuu-ic commented 3 years ago

I am mostly thinking along the lines of adding this as a configuration option which can be used with the serializeEx and deserializeEx methods. This allows you to use it only in those circumstances where you actually need it and it makes it a very deliberate choice. For all other cases you can keep the more sensible default.