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

to_model logic breaks down on nullable fields. #56

Open
andyDoucette opened this issue May 22, 2022 · 4 comments
Open

to_model logic breaks down on nullable fields. #56

andyDoucette opened this issue May 22, 2022 · 4 comments
Assignees

Comments

@andyDoucette
Copy link

I'm still trying to get all() to work with this resource.

As a work-around to that first issue, I'm using this code temporarily, although it totally isn't right (I shouldn't be allowing the making of models that are invalid).

    #This is used to store the beginning and end of a time tracking period for a given user on a given node.
    @kwdef mutable struct TTInterval <: AbstractModel
        id::DbId = DbId()
        #TODO:  When https://github.com/GenieFramework/SearchLight.jl/issues/55 is resolved, 
        # use these commented lines instead.  We shouldn't really be able to make models that aren't valid.
        #node_id::Int64
        #user_id::Int64
        #start_time::DateTime
        node_id::Int64=-1
        user_id::Int64=-1
        start_time::DateTime=DateTime(1900)

        #end_times can be nothing (aka NULL):
        end_time::Union{Nothing,DateTime}=nothing
    end

The issue I'm having now is with the end_time field.

julia> using TTIntervals

julia> all(TTIntervals.TTInterval)
[ Info: 2022-05-22 05:36:00 SELECT `ttintervals`.`id` AS `ttintervals_id`, `ttintervals`.`node_id` AS `ttintervals_node_id`, `ttintervals`.`user_id` AS `ttintervals_user_id`, `ttintervals`.`start_time` AS `ttintervals_start_time`, `ttintervals`.`end_time` AS `ttintervals_end_time` FROM `ttintervals` ORDER BY ttintervals.id ASC
┌ Error: 2022-05-22 05:36:02 MethodError(convert, (Union{}, Dates.DateTime("2023-01-01T00:00:00")), 0xffffffffffffffff)
└ @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:536
ERROR: MethodError: Cannot `convert` an object of type 
  Dates.DateTime to an object of type 
  Union{}
Closest candidates are:
  convert(::Type{Union{}}, ::Any) at essentials.jl:203
  convert(::Type{T}, ::Any) where T<:VecElement at baseext.jl:8
  convert(::Type{T}, ::T) where T<:Tuple at essentials.jl:315
  ...
Stacktrace:
  [1] convert(#unused#::Core.TypeofBottom, x::Dates.DateTime)
    @ Base ./essentials.jl:203
  [2] convert(#unused#::Type{Nothing}, x::Dates.DateTime)
    @ Base ./some.jl:36
  [3] oftype(x::Nothing, y::Dates.DateTime)
    @ Base ./essentials.jl:375
  [4] to_model(m::Type{TTIntervals.TTInterval}, row::DataFrames.DataFrameRow{DataFrames.DataFrame, DataFrames.Index}; skip_callbacks::Vector{Symbol})
    @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:534
  [5] to_model
    @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:492 [inlined]
  [6] to_model!!
    @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:572 [inlined]
  [7] to_models(m::Type{TTIntervals.TTInterval}, df::DataFrames.DataFrame)
    @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:473
  [8] find(m::Type{TTIntervals.TTInterval}, q::SearchLight.SQLQuery, j::Nothing) (repeats 2 times)
    @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:92
  [9] #all#34
    @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:179 [inlined]
 [10] all(m::Type{TTIntervals.TTInterval})
    @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:179
 [11] top-level scope
    @ REPL[2]:1

Digging into the SearchLight here's the relevant to_model() code in SearchLight.j:

491: function to_model(m::Type{T}, row::DataFrames.DataFrameRow; skip_callbacks::Vector{Symbol} = Symbol[])::T where {T<:AbstractModel}
....
498:         obj=m()  #Simplified for clarity
...
534:      setfield!(obj, unq_field, oftype(getfield(_m, unq_field), value))

So, basically, on line 498, SearchLight makes an empty struct, and then later it tries to set the fields using oftype(getfield(_m, unq_field), value). In other words, it looks at the type of each field in the initialized object and assumes that everything we ever put in that field will be of the same type. But, what about Unions? They can hold different types. So here we have a case where we initialized to nothing, but we tried to put a DateTime in it's place instead. That's totally valid according to the struct, but the way that to_model is making the assumption that "once a T, always a T" doesn't seem right when Unions are involved. I really hope I'm doing something dumb at this point, because being able to make a field in a database NULLable doesn't seem like a radical thing, yet everywhere I turn there's a new error message and more of other people's code to read to figure out why. 😢

@FrankUrbach
Copy link
Contributor

The issue is a little bit more difficult. You can have two situations. First, you design a struct without types. Then you are free to give back what you want. If you have a struct with defined types you are in trouble. There you can have nulls in your result set. If they aren't defined by Union type with Nothing as one type inside you will run into an error. A thought which comes to my mind would be to use a macro to extend the structs type. But that's not the best fit because somebody who wants to use the variables inside the struct directly don't want deal everywhere with Union types. So I think the only way would be to deal with user defined Union types which deal with fields which can be null in the result set. I think I've made something similar in the past. I will have a look at some code which was not accepted for an PR.

@essenciary
Copy link
Member

The issue is that union types are not proper types (or at least I'm not aware of reflection APIs for union types).

Let's take this example:
y::Union{Number,Nothing} = nothing

How do we determine the type of y?

julia> y::Union{Number,Nothing} = nothing

julia> typeof(y)
Nothing

-> Julia does not tell us that y is a Union.

So how can SearchLight know into what types to try and convert?

@FrankUrbach
Copy link
Contributor

Your example isn't chosen properly for the context we are in Searchlight. To determine the type of a field of a struct you have to look at the whole struct and then you will see the Union type of the field. If you try this on the level of the field itself it will fail as you stated above. In this context you are absolutely right. On the level of the field your example only says: "Numbers and Nothing are allowed", not more. I've done something similar in my package TypeDBORM where such question comes also in my mind. Determine Types in a Union isn't that hard. Union is build like a linked list. Each Union type has a field a and a field b. If there are more than two types b will be also a Union and the game begins again. So testing if there are only two types and one of them is Nothing isn't that hard. It took me 3 lines to get it work. Not nice, not fast, but it works.

@essenciary
Copy link
Member

Your example isn't chosen properly for the context we are in Searchlight. To determine the type of a field of a struct you have to look at the whole struct and then you will see the Union type of the field.

Can you show an example of this?

@AbhimanyuAryan AbhimanyuAryan self-assigned this Jun 13, 2022
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

No branches or pull requests

4 participants