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

"ERROR: UndefKeywordError: keyword argument _____ not assigned" when doing all(__) #55

Open andyDoucette opened 2 years ago

andyDoucette commented 2 years ago

Hello! I really am eager to see SearchLight become a fully-documented web framework for Julia, something the language desperately needs. The code looks very capable, it's just a challenge to find out how to use it correctly.

Anyway, here's the issue I'm having at the moment. I'm using SearchLight v2.2.1, but this also happened with v2.2.0.

I have a resource TTIntervals which defines the following struct and corresponding kwdef constructor:

@kwdef mutable struct TTInterval <: AbstractModel
    id::DbId = DbId()
    node_id::Int64
    user_id::Int64
    start_time::DateTime
    #end_times can be nothing (aka NULL):
    end_time::Union{DateTime,Nothing}=nothing
end

Here is the sql from sqldump that describes the resulting table in the database:

CREATE TABLE `ttintervals` (
  `id` int NOT NULL AUTO_INCREMENT,
  `node_id` int DEFAULT NULL,
  `user_id` int DEFAULT NULL,
  `start_time` datetime DEFAULT NULL,
  `end_time` datetime DEFAULT NULL,
  PRIMARY KEY (`id`),
  KEY `ttintervals__idx_node_id` (`node_id`),
  KEY `ttintervals__idx_user_id` (`user_id`)
) ENGINE=InnoDB AUTO_INCREMENT=13 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;

I have this data in the database added through SearchLight by making an instance of TTInterval and save!(ing) it:

mysql> select * from ttintervals;
+----+---------+---------+---------------------+---------------------+
| id | node_id | user_id | start_time          | end_time            |
+----+---------+---------+---------------------+---------------------+
|  1 |       1 |       1 | 2022-01-01 00:00:00 | 2023-01-01 00:00:00 |
|  2 |       1 |       1 | 2022-01-01 00:00:00 | 2023-01-01 00:00:00 |
|  3 |       1 |       1 | 2022-01-01 00:00:00 | 2023-01-01 00:00:00 |
|  4 |       1 |       1 | 2022-05-03 14:32:05 | 2023-01-01 00:00:00 |
|  5 |       1 |       1 | 2022-05-03 14:32:05 | 2023-01-01 00:00:00 |
|  6 |       1 |       1 | 2022-05-03 14:32:05 | 2023-01-01 00:00:00 |
|  7 |       1 |       1 | 2022-05-03 14:32:05 | NULL                |
|  8 |       1 |       1 | 2022-05-03 14:32:05 | 2023-01-01 00:00:00 |
|  9 |       1 |       1 | 2022-05-03 14:32:05 | 2023-01-01 00:00:00 |
| 10 |       1 |       1 | 2022-05-03 14:32:05 | NULL                |
| 11 |       1 |       1 | 2022-05-03 14:32:05 | NULL                |
| 12 |       1 |       1 | 2022-05-03 14:32:05 | NULL                |
+----+---------+---------+---------------------+---------------------+
12 rows in set (0.00 sec)

The issue comes when I try to retrieve the data. It doesn't matter of I do a find() or a all(), anything that tries to convert DataTables into objects results in the same error. Here it is:

julia> using SearchLight

julia> all(TTIntervals.TTInterval)
[ Info: 2022-05-22 01:19:18 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: UndefKeywordError: keyword argument node_id not assigned
Stacktrace:
 [1] TTIntervals.TTInterval()
   @ TTIntervals ./util.jl:453
 [2] 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:493
 [3] to_model
   @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:492 [inlined]
 [4] to_model!!
   @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:572 [inlined]
 [5] to_models(m::Type{TTIntervals.TTInterval}, df::DataFrames.DataFrame)
   @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:473
 [6] find(m::Type{TTIntervals.TTInterval}, q::SQLQuery, j::Nothing) (repeats 2 times)
   @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:92
 [7] #all#34
   @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:179 [inlined]
 [8] all(m::Type{TTIntervals.TTInterval})
   @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:179
 [9] top-level scope
   @ REPL[4]:1

The stack trace at [2] points to this code:

@ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:493

function to_model(m::Type{T}, row::DataFrames.DataFrameRow; skip_callbacks::Vector{Symbol} = Symbol[])::T where {T<:AbstractModel}
  _m::T = try
->  m()
  catch ex
    isa(ex, MethodError) ? Base.invokelatest(m) : rethrow(ex)
  end

It seems that SearchLight is trying to create an instance of the model without providing any kwargs at all and that's erroring out. It's in a try though so I'm not really sure why it's erroring out, but it is.

It seems like putting something in the DB and taking it out again is pretty normal stuff for an ORM, so I'm baffled that I would be the only one having this issue. Am I doing something dumb or is the package broken?

andyDoucette commented 2 years ago

Also, is this the right way to define fields that can be NULL (TTInterval.end_time)?

essenciary commented 2 years ago

@andyDoucette it's been quite a few years since this was developed but I'm pretty sure the issue is that SearchLight does not yet support Union types. The code was actually written before Union{Nothing,T} became the way to represent nullables. For some reason this problem rarely comes up so nobody actually worked on it.

So the issue is that we need to extend the logic that retrieves the data from the DB in whatever Julia type it comes, and then convert it to the Union type specified by the model's field.

andyDoucette commented 2 years ago

Thank you for responding. I changed the definition to the following as a sloppy work-around and it still does not work:

    #We need to do this because SearchLight does not yet support nullable fields:
    dateRepresentsNotDefined=DateTime(1000)

    #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()
        node_id::Int64
        user_id::Int64
        start_time::DateTime
        end_time::DateTime=dateRepresentsNotDefined
    end

It's the same error, happening when pulling data from the database and trying to make a model of it. The system tries to make an empty structure first and errors out there because my model requires a value for each field. I really don't want to have to set defaults because that implies that it's "ok" to make a model that's partially undefined.

julia> a=TTIntervals.TTInterval(node_id=1,user_id=1,start_time=DateTime(2020)) 
julia> save(a)
[ Info: 2022-05-24 23:07:04 INSERT INTO ttintervals ( `node_id`, `user_id`, `start_time`, `end_time` ) VALUES ( 1, 1, '2020-01-01T00:00:00', '1000-01-01T00:00:00' )
true
julia> all(TTIntervals.TTInterval)
[ Info: 2022-05-24 23:09:12 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: UndefKeywordError: keyword argument node_id not assigned
Stacktrace:
 [1] TTIntervals.TTInterval()
   @ TTIntervals ./util.jl:453
 [2] 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:493
 [3] to_model
   @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:492 [inlined]
 [4] to_model!!
   @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:572 [inlined]
 [5] to_models(m::Type{TTIntervals.TTInterval}, df::DataFrames.DataFrame)
   @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:473
 [6] find(m::Type{TTIntervals.TTInterval}, q::SQLQuery, j::Nothing) (repeats 2 times)
   @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:92
 [7] #all#34
   @ ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:179 [inlined]
 [8] all(m::Type{TTIntervals.TTInterval})
   @ SearchLight ~/.julia/packages/SearchLight/2kAM0/src/SearchLight.jl:179
 [9] top-level scope
   @ REPL[17]:1
essenciary commented 2 years ago

I've had a very long discussion yesterday on the Julia slack about this. It really seems that there is no easy way around it. There are a multitude of factors, including how Julia handles Union types, the fact that the Julia type has no equivalent in the DB types, etc.

Additionally it's also a reflection of the design decisions within SearchLight. Indeed, the requirement is that SearchLight is able to create a default instance of the struct. I think it's because SearchLight dev started a long time ago where there was no easy way to pass a number of unknown kwargs (which was solved by the introduction of NamedTuples later on).

Based on the discussion I've had, one potential solution is to switch to (or add) logic to first attempt to instantiate the specific object, by invoking the constructor and passing the kwargs. This might allow the compiler to construct the correct type of the Union. But it's not guaranteed as the union should not be ambiguous. I expect that it will work to express nullable types as Union{T,Nothing} but won't work for more complex Unions. But it's worth a try.

Beyond this, the recommendation was to go with user defined constructors and user defined convert/parse methods.

I can take a quick try at refactoring SearchLight to directly construct the object using the values retrieved from the DB. But to make it easier for me, can you please share a Julia file with a MWE I can use as test? Model definition and SQLite DB to replicate the issue and use as failing/passing test? Thanks.