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

Disallow referencing by reference instance #92

Open
vladfaust opened this issue Apr 14, 2019 · 1 comment
Open

Disallow referencing by reference instance #92

vladfaust opened this issue Apr 14, 2019 · 1 comment
Labels
breaking Breaking change rfc Request For Comments

Comments

@vladfaust
Copy link
Member

Currently this code can raise in runtime with NilAssertionError if user.id is nil.

class Use
  schema users do
    pkey id : Int32
  end
end

class Post
  schema posts do
    type author : User, key: "the_author_id"
  end
end

Post.where(author: user)
Post.update.set(author: user)

I propose to make things more explicit but reduce runtime errors in exchange. The .id referenced in queries is User variable, not its column name. The code would raise in compilation time if something is wrong:

Post.where("author.id": user.id!) # SELECT FROM posts WHERE the_author_id = ?
Post.update.set("author.id": user.id!) # UPDATE posts SET the_author_id = ?

This could create confusion between explicit SQL variants, though. Such a query would raise in runtime, because posts table doesn't have author.id column in the database schema:

Post.set("author.id", user.id!) # UPDATE posts SET author.id = ?

Possible workaround is bang query methods which mean explicit SQL, i.e.:

Post.set!("the_author_id", user.id)
User.where!("id IN ?, ?", {41, 42})
@vladfaust vladfaust added rfc Request For Comments breaking Breaking change labels Apr 14, 2019
@vladfaust vladfaust added this to the 0.8.0 milestone Apr 16, 2019
@vladfaust vladfaust removed this from the 0.9.0 milestone Apr 27, 2019
@vladfaust
Copy link
Member Author

Another approach is to restrict reference types to their primary key types:

Post.set(author: user.id!)

Which makes the most sense IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change rfc Request For Comments
Projects
None yet
Development

No branches or pull requests

1 participant