crystal-lang / crystal-db

Common db api for crystal
https://crystal-lang.github.io/crystal-db/api/latest/
MIT License
303 stars 55 forks source link

`DB::Serializable` deserialization breaks at compile time if a property type includes a `DB` type #205

Open jgaskins opened 8 months ago

jgaskins commented 8 months ago

I have the concept of a Transaction in my app, and when I added a property whose type included that name (in my case it was a Transaction::Status enum), it stopped compiling with this error message:

 > 40 |                   __temp_2301 =
 > 41 |
 > 42 |                       rs.read(Transaction::Status)
                                      ^------------------
Error: undefined constant Transaction::Status

But it very clearly exists in my model file. It turns out, the issue is that DB::Serializable sees Transaction and the Crystal compiler interprets that as DB::Transaction, but the error message doesn't make that clear. I assume this would also be the case if you had top-level types like Error or Driver.

require "pg"

pg = DB.open("postgres:///")

pg.query_all <<-SQL, as: Transaction
  SELECT
    gen_random_uuid() id,
    0 status
  FROM generate_series(1, 10) AS transactions
  SQL

struct Transaction
  include DB::Serializable

  getter id : UUID
  getter status : Status

  enum Status
    Pending
    Complete
    Canceled
  end
end
bcardiff commented 8 months ago

I'm surprised this didn't came up before. The following snippet using JSON::Serializable also suffers from this

require "json"

struct Builder
  include JSON::Serializable
  @a : Int32?
end

class A
  include JSON::Serializable
  @builder : Builder
end

pp! A.from_json(%<{"builder:{"a":1}}>)
 > 33 |                    begin 
 > 34 |                     
 > 35 |                       ::Union(Builder).new(pull)
                                                   ^---
Error: expected argument #1 to 'JSON::Builder.new' to be IO, not JSON::PullParser
ysbaddaden commented 8 months ago

I regularly trip on that. The code generated by macros won't be interpreted in the context of the macro but where the code is generated, which makes sense but isn't always obvious.

I started to always use ::Full::Path::KlassName in macros to avoid these.

bcardiff commented 8 months ago

That would be the dual, a type name written in a macro expands at call site and suddenly it binds to something declared in the call site.

In this situation we are, for some reason, resolving the type name in the context of the macro (Builder binds to JSON::Builder, or Transaction to DB::Transaction) and I think there is nothing the user could do to prevent this.

In your use case as a macro author you can choose to use fully qualified names. But here even declaring @builder : ::Builder does not help 😢 .

Ideally I would like for Path in macro to be resolved at call-site, but at least have a to_fully_qualified_id method in macros would be a reasonable workaround.

@jgaskins proposal fix is partial. What happens if the type is generic? How we can fully qualify the type arguments (in a reasonable way)? If we bake a to_fully_qualified_id then we will have a better fix than #206. And if we have that function, why is not the default behavior?

jgaskins commented 8 months ago

This is a good point. Should we instead open this issue on the Crystal compiler? I opened it here because for some reason I hadn't considered that other X::Serializable mixins would also run into this issue, though in hindsight that makes perfect sense and I don't think each one should have to deal with it separately.