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

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

Open
jgaskins opened this issue Mar 10, 2024 · 4 comments · May be fixed by #206
Open

Comments

@jgaskins
Copy link
Contributor

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
@jgaskins jgaskins linked a pull request Mar 10, 2024 that will close this issue
@bcardiff
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

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.

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

Successfully merging a pull request may close this issue.

3 participants