has_many/belongs_to breaks with an underscore

[mcasper]

The has_many/belongs_to codegen annotations don't seem to be able to handle a table name consisting of multiple words and underscores, as it smashes all the words together when searching for the module name. The following code fails to compile:

#![feature(plugin, custom_derive)]
#![plugin(diesel_codegen)]

#[macro_use]
extern crate diesel;

fn main() {}

#[has_many(soccer_balls)]
pub struct User {
    id: i32
}

#[belongs_to(user)]
pub struct SoccerBall {
    id: i32,
    user_id: i32
}

table! {
    users {
        id -> Serial,
    }
}

table! {
    soccer_balls {
        id -> Serial,
        user_id -> Integer,
    }
}

Giving the following error:

src/main.rs:1:1: 1:1 error: failed to resolve. Use of undeclared type or module `soccerballs` [E0433]
src/main.rs:1 #![feature(plugin, custom_derive)]
              ^
src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0433` to see a detailed explanation
src/main.rs:1:1: 1:1 error: use of undeclared type name `soccerballs::table` [E0412]
src/main.rs:1 #![feature(plugin, custom_derive)]
              ^
src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0412` to see a detailed explanation
src/main.rs:1:1: 1:1 error: failed to resolve. Use of undeclared type or module `soccerballs` [E0433]
src/main.rs:1 #![feature(plugin, custom_derive)]
              ^
src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0433` to see a detailed explanation
src/main.rs:1:1: 1:1 error: use of undeclared type name `soccerballs::user_id` [E0412]
src/main.rs:1 #![feature(plugin, custom_derive)]
              ^
src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0412` to see a detailed explanation
src/main.rs:1:1: 1:1 error: failed to resolve. Use of undeclared type or module `soccerballs` [E0433]
src/main.rs:1 #![feature(plugin, custom_derive)]
              ^
src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0433` to see a detailed explanation
src/main.rs:1:1: 1:1 error: unresolved name `soccerballs::table` [E0425]
src/main.rs:1 #![feature(plugin, custom_derive)]
              ^
src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0425` to see a detailed explanation
src/main.rs:1:1: 1:1 error: failed to resolve. Use of undeclared type or module `soccerballs` [E0433]
src/main.rs:1 #![feature(plugin, custom_derive)]
              ^
src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0433` to see a detailed explanation
src/main.rs:1:1: 1:1 error: unresolved name `soccerballs::user_id`. Did you mean `self.soccerballs::user_id`? [E0425]
src/main.rs:1 #![feature(plugin, custom_derive)]

// Lots more of those...

src/main.rs:14:1: 14:20 note: in this expansion of #[belongs_to] (defined in src/main.rs)
src/main.rs:1:1: 1:1 help: run `rustc --explain E0412` to see a detailed explanation
error: internal compiler error: ty_is_local invoked on unexpected type: [type error]
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: run with `RUST_BACKTRACE=1` for a backtrace
thread 'rustc' panicked at 'Box<Any>', ../src/libsyntax/errors/mod.rs:469

The expansion is looking for soccerballs::table and friends, when it should be looking for soccer_balls::table and friends.

[sgrif]

In addition to just flat out fixing this problem, I'd also like to decouple the table name from the struct name, and allow a #[table_name="foo"] annotation.

[mcasper]

The decoupling would probably help a lot, as it looks like it has trouble with any edge case pluralization as well

[sgrif]

Yeah, it literally is .to_lowercase + s right now. We can make it
slightly better, but I don't want to try and handle actual pluralization
for every word

On Mon, Jan 11, 2016, 5:46 PM Matt Casper notifications@github.com wrote:

The decoupling would probably help a lot, as it looks like it has trouble
with any edge case pluralization as well

—
Reply to this email directly or view it on GitHub
#86 (comment).

[mfpiccolo]

👍 for #[table_name="foo"]

[sgrif]

Also I didn't mention it earlier, but also keep in mind that I've left associations out of the public API of the released crate, as I'm not happy with the current design and it may change further (it also likely requires the lattice rule coming w/ specialization to implement properly)

[sgrif]

So I've been starting to think more heavily about this, and specifically how to avoid duplicating a lot of information when dealing with non-standard tables and primary keys. An interesting thing about the #[table_name] annotation is that I can't actually access it from annotations on other items (which makes sense, especially when considering the associated struct might be coming from another crate).

Just spitballing some thoughts here. Scoping to belongs_to only to simplify. We might be able to work this into assuming the other side implements a trait, rather than how we operate today. If we do #[belongs_to(bar)] Struct Foo;, then the code that'll get generated is:

impl ::diesel::BelongingToDsl<Bar> for Foo {
    type Output = ::diesel::helper_types::FindBy<
        foos::table,
        foos::bar_id,
        i32,
    >;

    fn belonging_to(model: &Bar) -> Self::Output {
        foos::table.filter(foos::bar_id.eq(model.id.clone()))
    }
}

impl ::diesel::JoinTo<bars::table> for foos::table {
    fn join_sql(&self, out: &mut ::diesel::query_builder::QueryBuilder)
        -> ::diesel::query_builder::BuildQueryResult
    {
        try!(bars::table.from_clause(out));
        out.push_sql(" ON ");
        foos::bar_id.eq(bars::table.primary_key()).to_sql(out)
    }
}

So I guess really all we need is something like

trait Identifiable {
    type Table: Table;
    type Pk: AsExpression<Self::Table::PrimaryKey::SqlType>;

    fn table() -> Self::Table;
    fn primary_key(&self) -> Self::Pk;
}

Now the question is where do we actually generate this. Are we fine with just requiring a #derive(Identifiable) on the other side? I could just tack it onto something common like Queriable, but that would imply that you don't have models which aren't associated with a single row of a single table.

It's worth noting that all of this is focusing around the idea that these structs will represent a single row on a single table. While that's counter to the design of the framework overall, I think that more or less has to be a given when we start talking about associations (unless we want to explore the idea that you'd have a single struct to represent both sides, but I don't think that'll work well with one-to-many, and would inhibit code re-use).

It's also worth noting that we never actually use the argument to belongs_to, other than to generate the foreign key and infer the struct name.

With all of this in mind, I'm thinking that we should change the API to #[belongs_to(User)], with the ability to specify the foreign key: #[belongs_to(User, foreign_key="author_id")]. With this change, the generated code would become:

impl ::diesel::BelongingToDsl<Bar> for Foo {
    type Output = ::diesel::helper_types::FindBy<
        foos::table,
        foos::bar_id,
        i32,
    >;

    fn belonging_to(model: &Bar) -> Self::Output {
        foos::table.filter(foos::bar_id.eq(<Bar as Identifiable>::primary_key(model)))
    }
}

impl ::diesel::JoinTo<<Bar as Identifiable>::Table> for foos::table {
    fn join_sql(&self, out: &mut ::diesel::query_builder::QueryBuilder)
        -> ::diesel::query_builder::BuildQueryResult
    {
        try!(<Bar as Identifiable>::table().from_clause(out));
        out.push_sql(" ON ");
        foos::bar_id.eq(<Bar as Identifiable>::table().primary_key()).to_sql(out)
    }
}

This does mean that you can't have an association using a primary key other than that of the table. I think that's fine, as the point is that we're equating with a single row, and I've never seen a good use case for changing the primary key of an association in Rails.

This will be a little bit more murky when it comes to has_many, as that means the API becomes the slightly awkward #[has_many(Post)] or #[has_many(Post, foreign_key="post_id")]. It's worth noting that has_many as it is today is effectively worthless (other than enabling joining).

At some point it will also be the thing that makes you able to get Vec<(User, Vec<Post>)> out of a query, but I'm not sure that needs to care about the struct as much as it is a property of how you're performing the join. I was writing some examples of what I mean but as I wrote them I realized there's some flaws involving how it gets mixed with select. I'd have to implement to figure that out. But what I'm getting at is that unlike belongs_to, which does deal with a specific struct, has_many doesn't necessarily need to know about it, and in fact it is more about the table, implying that #[has_many(posts)] is in fact the right thing to do.

Perhaps at the heart of this is figuring out exactly what associations are meant to do. I do like Post::belonging_to(user) as a helpful piece of sugar over the common posts.filter(user_id.eq(user.id)) (I also just realized that could totally be posts::belonging_to(user) since the Post struct has nothing to do with belonging_to, but I'm struggling to come up with any cases where we need to involve structs.

If that's the case, maybe we need to separate out the relationships between tables from the relationships between structs. The biggest problem there is that we no longer have a place for you to actually annotate a table. I need to spike some more on what I want to be able to do with has_many to figure out the right place to solve this...