GenieFramework / SearchLight.jl

ORM layer for Genie.jl, the highly productive Julia web framework
https://genieframework.com
MIT License
139 stars 16 forks source link

Allow Union types as model fields #25

Closed CarpeNecopinum closed 2 years ago

CarpeNecopinum commented 4 years ago

It would be interesting to allow the use of models like:

@kwdef mutable struct Book <: AbstractModel
    ### INTERNALS
    _table_name::String = "books"
    _id::String = "id"

    ### FIELDS
    id::DbId = DbId()
    title::String = ""
    author::String = ""
    cover::Union{String,Missing} = missing
end

Where the cover is effectively optional and by default missing. Unfortunately, https://github.com/GenieFramework/SearchLight.jl/blob/4e001f2044f5c64731c78545644db0ffe5871b16/src/SearchLight.jl#L394 blocks this from working, as oftype(Book.cover, x) will call convert(Missing, x) which doesn't work for x being e.g. a String.

This PR changes that behavior in that we convert to the type of the field, rather than the type of its default value. The added function _union_convert handles cases like converting a Float64 to Union{Int,Nothing}, which would not work with Base.convert.

essenciary commented 3 years ago

@CarpeNecopinum Sorry, OMG, I missed this for so long! Is this necessary? Doesn't Julia handle the types if we just define the union, supporting all the types in the union?

CarpeNecopinum commented 3 years ago

I haven't used Genie for like half a year now, so I can't say how it is now for sure. But at the time of writing, SearchLight would always try to convert values read from the database into the concrete type (in the above example Missing) rather than the field type (Union{String,Missing}).

The _union_convert then is a second step that ensures better backwards compatibility, because you can't convert e.g. a Float64 into a Union{Int,Missing}, but you can convert it to Int. But that second part is basically optional, wheras the first part (https://github.com/GenieFramework/SearchLight.jl/pull/25/files#diff-15442cb6a676563c8cd8d8903211bbd55fadcdfeb0eca3f6bcd493cef2ac1d4cL700) is needed to support Union fields.

UniqueTokens commented 2 years ago

+1 Union types as model fields would be particularly useful for is_nullable columns in PostgreSQL. Any news on this?

essenciary commented 2 years ago

Related to current discussions here: https://github.com/GenieFramework/SearchLight.jl/issues/55 Closing this issue in favour of the newer one.