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

Feature : derive(Insertable) into multiple tables #1314

Closed
ghost opened this issue Nov 28, 2017 · 13 comments · Fixed by #3535
Closed

Feature : derive(Insertable) into multiple tables #1314

ghost opened this issue Nov 28, 2017 · 13 comments · Fixed by #3535

Comments

@ghost
Copy link

ghost commented Nov 28, 2017

I'm currently trying to use the same struct in many tables and was looking for something like this:

#[derive(Insertable)]
#[table_name="table1", "table2"]

instead of having to make separate structs for each table. Keep up the good work!

@sgrif
Copy link
Member

sgrif commented Nov 28, 2017

Ironically this used to work, but was removed when insertable_into was changed to derive(Insertable_. This has come up enough times that it's worth pursuing (I don't like the idea of giving multiple arguments to table_name, I think this needs a more explicit API.) Happy to do the legwork of implementing if someone proposes an API that seems reasonable.

Since this is not a feature that will require a breaking change, it is on hold until after 1.0.

@ghost
Copy link
Author

ghost commented Nov 29, 2017

Thanks for the response.
The first thing I tried was actually multiple table_name statements - maybe this is more intuitive:

#[derive(Insertable)]
#[table_name="table1"]
#[table_name="table2"]

@jeffw387
Copy link

jeffw387 commented Apr 1, 2019

I tried that as well before finding this issue. Seems reasonable to me.

@sgrif
Copy link
Member

sgrif commented Jun 27, 2019

I think that API is decent, but it has a few problems. The biggest one is that not all of our derives can handle more than one table (Identifiable semantically can't, AsChangeset can't because of its definition).

I'm fine with not being able to derive Identifiable on a struct that has multiple table names, as that'd be pretty nonsense anyway. Not being able to derive AsChangeset on the same struct is unfortunate though, as these two traits often go together.

Master is currently tracking 2.0, so we can technically make this change if we want (though it'd need a proposal on discourse following the template like all other breaking changes), but the big question is whether it's worth it.

All our derives that look at #[table_name] will need to be updated to handle multiple tables if they can, or error if not. It's probably worth getting a list of all affected derives before making the final call on what to do with AsChangeset, or if we should just go with a different, insertable specific API

@Boscop
Copy link

Boscop commented Mar 14, 2021

Any update on this? :)
I just ran into this issue, when I tried to the pattern that @Zsarl posted above (multiple table_name attributes).

@weiznich
Copy link
Member

@Boscop We are happy to receive a fully formulated feature request for this, otherwise as always: There is no update on an issue as the issue contains no update. That won't change by asking there, so please do ask such questions.

@toxeus
Copy link
Contributor

toxeus commented Jan 18, 2022

@weiznich I'm willing to take a stab on this. In light of the upcoming 2.0 release, is there a preferred way of defining multiple table names (attribute list vs multiple attributes vs something else) as long as we have the chance to make breaking changes? If not then I'd go for the attribute list #[table_name = foo_table, bar_table].

@weiznich
Copy link
Member

@toxeus The blocking issue here is to figure out how this interacts with other derives that use the table name. How the attribute should look like is a rather minor question.

@toxeus
Copy link
Contributor

toxeus commented Jan 18, 2022

I see, multiple derive macros share the same table_name attribute. The trivial solution would be to add a new table_names (not the s) attribute that is only used by the Insertable derive and accepts a list of tables. If backwards-compat is desired then it would fall back to table_name if table_names is not found.

@weiznich
Copy link
Member

@toxeus I think I would be fine with the following approach:

We allow both: Either #[diesel(table_name = abc, table_name = dcf)] or #[diesel(table_name = abc)] and #[diesel(table_name = cdf) to be present on the same struct. That gets collected internally in a Vec<_> by the derive crate. Now Insertable is able to emit more than one impl, while any other derive should emit an error message that multiple table name attributes are not supported. I would accept a PR for that if it includes:

  • Compile tests showing the behaviour with other derives
  • A test for the insertable case
  • Documentation for the insertable case.

@jakob-lilliemarck
Copy link

..is it possible to achieve this in the current version if implementing Insertable manually on the struct (not using the macro)? I've also been trying to achieve this, any pointers or updates to this request would be much appreciated

@fawdlstty

This comment was marked as off-topic.

@weiznich

This comment was marked as off-topic.

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

Successfully merging a pull request may close this issue.

7 participants