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

Cryptic error when passing int literals to foreign key columns in create #426

Open
ghost opened this issue Sep 26, 2020 · 5 comments
Open

Comments

@ghost
Copy link

ghost commented Sep 26, 2020

Granite::Connections << Granite::Adapter::Pg.new(name: "db", url: "postgres://test@localhost/test")
require "granite/adapter/pg"

class Post < Granite::Base
  connection db
  table posts
  column id : Int64, primary: true
  column title : String
  belongs_to :user
end
class User < Granite::Base
  connection db
  table users
  column id : Int64, primary: true
  column email : String
  column name : String
  has_many :post
end

u = User.create!(email: "[email protected]", name: "Bob")
p = Post.create!(title: "Hi", user_id: u.id)

The above works fine. But if I switch u.id for a literal number on the last line, even if it's the correct number, I get:

Unhandled exception: Could not process Post (Granite::RecordNotSaved)
  from lib/granite/src/granite/transactions.cr:28:9 in 'create!'
  from lib/granite/src/granite/transactions.cr:21:7 in 'create!:title:user_id'
  from src/main.cr:24:1 in '__crystal_main'
  from ../../../../usr/lib/crystal/crystal/main.cr:105:5 in 'main_user_code'
  from ../../../../usr/lib/crystal/crystal/main.cr:91:7 in 'main'
  from ../../../../usr/lib/crystal/crystal/main.cr:114:3 in 'main'
  from __libc_start_main
  from _start
  from ???

I was very confused by this.

@Blacksmoke16
Copy link
Contributor

What's the error if you don't use! and then do pp p.errors.

@ghost
Copy link
Author

ghost commented Sep 26, 2020

[#<Granite::ConversionError:0x7ff6a3bdaba0
  @field="user_id",
  @message="Expected user_id to be (Int64 | Nil) but got (Int32 | String).">]

Did not know that feature existed.

If I change the id columns to both be Int32, the same error occurs (it still expected user_id to be Int64 | Nil), but if I also add foreign_key: user_id : Int32, to Post, it fixes it. It also fixes it if I don't change the models, but cast the literal to Int64.

So it's a type error. Can this not be caught at compile time?

Also curious why it thinks it got Int32 | String, but merely changing Int32 to Int64 fixed it.

@ghost
Copy link
Author

ghost commented Sep 26, 2020

Oh, I see why it isn't caught at compile time. In src/granite/transactions.cr:9, the args are caught with a double splat and then collected into a hash.

I wonder if there's a better way to implement that. It also must be why it doesn't error when you pass nonexistent fields to create.

@ghost
Copy link
Author

ghost commented Sep 26, 2020

I see what caused the error to be so cryptic. save in transactions.cr just returns false if it failed due to a validation error, not preserving the error message unless you inspect with .errors.

@crimson-knight
Copy link
Member

I've run into this issue when using other ORMs too, it's an inconvenience of the language by default having numbers be Int32 but the DB expecting Int64.

You'd have to do something like u.id.to_i64 which works, but feels like this shouldn't be a hurdle.

There are some types like this that just feel like they should be detected and then properly cast to the right type without having to specify it everywhere in the code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants