-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow to ignore field of struct #860
Comments
I'd call it |
Umm, I don't know if I explain correctly the idea, maybe the serde annotation in my code confuses more than helps. What I was searching is a way to totally ignore that field in the database. That means, Diesel to load (in that case) always None in the Other question than can help. Having this struct: #[derive(Clone, Debug, Queryable, Serialize, AsChangeset, Identifiable, Associations)]
#[has_many(authors)]
pub struct User {
pub id: i32,
pub email: String,
pub author: Vec<Author>
} is possible to have something like |
Oh, yes, I think that's what I understood as well. I was suggesting
Did I get that right?
Currently, it's possible to get |
Ok, you understand correctly :) Thanks! I will check this frequently! |
I've definitely warmed to the idea of marking a field as ignorable for update/insert (to skip a field for update, you can always use my hilarious, hacky workaround for now. I'm not sure I see the use case for skipping something on |
I think the only real use case for skipping a field on Using the example code from the association docs: let user = try!(users::find(1).first(&connection));
let posts = Post::belonging_to(&user).load(&connection); then you would make posts a proper child of user: user.posts = posts; The docs say that you could pass it around as {
"id": 0,
"name": "Foo",
"posts": []
} But passing it around as a tuple would have it represented as: {
"user": {
"id": 0,
"name": "Foo"
},
"posts": []
} |
My use case. I have a struct: #[derive(Deserialize, AsChangeset)]
#[table_name = "users"]
pub struct UpdateUserData {
username: Option<String>,
email: Option<String>,
bio: Option<String>,
image: Option<String>,
#[skip_somehow_plz]
password: Option<String>,
} I use this struct to parse POST params and then I'd like to use it as a changeset. So I want to skip |
I have the same use case as @TatriX, the reason I want the field to be in the same struct as |
As far as I know this depends on #1512 #[derive(Queryable)]
struct Foo {
id: i32,
name: String,
bar: Bar,
}
#[derive(Queryable)]
struct Bar {
id: i32,
name: String
}
let r: Vec<Foo> = foo::table.inner_join(bar::table).select((foo::id, foo::name, (bar::id, bar::name)).load(conn)?; I'm ready to take this issue if you think this should be implemented. |
It's not blocked on that PR. It just hasn't been a priority. As I mentioned in #860 (comment) though, if/when this lands, it'll be for |
It has a good use case to be implemented on |
The link is broken. Could you repeat your solution? I have a structure with only one field other in update and create action (e.g. upsert action), and I want to use one struct with more than 40 fields in two cases. |
@mikhail-krainik That was this thing. But be warned: I'm quite sure doing that is a bad idea… |
I believe I have another use case where it would be helpful for marking fields of a struct to be ignored for a derived For example, most of my tables track when they were created and the most recent modification time in two columns,
|
@TrevorFSmith This would be lovely, or maybe detect |
Interested in this feature. My use case is that I want to take a nested JSON object returned by an API and generate a number of insertable structs. I want the parent struct to have the child struct as a Vec field so I can deserialize the JSON, but that makes it so I can't insert the struct into the database. |
Another use case: Reuse of objects between Diesel and Juniper |
@dyst5422 I've written wundergraph for this use case as you have to think about much more than a few field attributes for providing a graphql API that does not suffer from N+1 query problems. The normal juniper API generates N+1 queries for a nested entity. |
I have to add some |
@tuxzz Just commenting here that you need a certain feature will help that the feature appears in a magical way. You either need to implement all traits manually, which would allow you to do whatever you want there, or sit down and implement that feature for diesel or find a other solution. |
TL;DR: I have an initial implementation of something that could address this issue. I think there are a number of user cases that are fairly common throughout different projects. However, it is often the case that the struct that you get from DB needs to be "augmented" with additional information before it becomes complete, and that process would (ideally) be lazy in nature. It would be really useful if the more experienced Diesel users could share their best practices to overcome the challenge described above. It could alleviate the problem of not having optional fields and, most importantly, show me some proper Rust coding practices! :-) So, now that the long and winded intro is out of the way, I've actually gone ahead and created my version of Queryable that allows for optional fields. It looks something like this: #[derive(PartialQueryable, Serialize)]
pub struct Account {
pub id: u32,
pub name: String,
#[table_name = "group_acc"]
#[column_name = "group_id"]
pub id_of_group = u32,
// ... other fields ...
#[diesel(skip)]
pub activity_dates: Vec<AccActivityDates>
} For now I'm calling it PartialQueryable and it allows you to add a account::table
.inner_join(group_acc::table)
.filter(account::id.eq_any(acc_ids))
// This is equivalent to (accounts::id, accounts::name, ...) - `activity_dates` not included
.select(Account::ALL_COLUMNS)
.load::<Account>(&conn)
.expect("Something went wrong.") This is working in my own project, but I have to clarify that I'm only using it in one place at the moment. In fact, it includes some additional options, which you might have noticed in the example. My implementation is fairly basic and probably doesn't cover a number of corner cases. Things might (will?) start to fall apart if you start doing fancy stuff with these additional "powers". At the moment this is just on a local repo that's simply a manual copy of enough code from Queryable to make it work. But I could prepare it for a PR if it's going to be considered. Cheers! |
@alfonso-eusebio As a general note at the top: As pointed out in this comment (#860 (comment)) and this PR (#1582) we are not interested in having an option to ignore fields for
#[derive(Queryable, Serialize)]
struct User {
id: i32,
name: String,
// …
}
#[derive(Serialize)]
struct OuterUser {
#[serde(flatten)]
user: User
other_additional_field: Foo,
}
// serializing OuterUser results in
// { "id": 42, "name": "John", "other_additional_field": {}} (This case already covers >90% of the use cases of the requested feature in my experience)
The following section addresses specific parts of the comment above:
I'm not sure where exactly additional cloning is involved to include additional information. The workflow with your proposed extension is roughly the following: #[derive(PartialQueryable, Serialize)]
pub struct Account {
pub id: u32,
pub name: String,
// ... other fields ...
#[diesel(skip)]
pub activity_dates: Vec<AccActivityDates>
}
let mut db_result = accounts::table.load::<Account>(&conn)?;
for a in &mut db_result {
// just a unrelated side note:
// if this accesses the database you get a N+1 query bug
a.get_activity_dates();
} while using the built in API + serde gives you this: #[derive(Queryable, Serialize)]
pub struct Account {
pub id: u32,
pub name: String,
}
#[derive(Serialize)]
pub struct JsonAccount {
#[serde(flatten)]
pub user: Account,
pub activity_dates: Vec<AccActivityDates>
}
let db_results = accounts::table.load::<Account>(&conn);
db_results.into_iter().map(|user| {
// same about N+1 queries applies here
// but if you need to load stuff from the database here
// you can now use the `diesel::associations` api to separately load
// those data at once and then use `.zip()` instead to merge the lists
JsonAccount {
user,
activity_dates: get_activity_dates()
}
}).collect();
That is a completely separate feature in my opinion. In fact there is already something like that in development with #2367. I would appreciate feedback about the usability of this API there. Also if someone comes up with good idea that does not require a separate query method to implement this feature that would also be fantastic. |
@weiznich I have a different use-case that may merit discussion. I am running into a ResourceExhausted error on a rather large query for historical transactions in my application:
I believe this is because of a I opened this discussion (#3379) because I would like to write a new queryable type that does not have the raw_json, or somehow set it to none within the query such that it is not actually pulled over from the database. Also, is the default GRPC max of 16 megabytes configurable at all? |
Your issue seems unrelated to this, as implementing this is not a requirement to solve it: #3379 (comment). gRPC discussions are unrelated to diesel, but this is a matter of both client and server configuration. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Are there any updates? It really is a very useful feature. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as off-topic.
This comment was marked as off-topic.
@weiznich @Ten0 To give some context currently I have a struct for each 'action' Create, Read, Update and Delete these structs are used internally for interacting with the database. Now to expose this data to for example an end user via an API I want to omit the ID field that is used by the database. There are multiple solutions for this problem one of them is to create a different struct that doesn't have this ID field present and with a From/Into this can be done pretty elegant. Because the API exposes this data via JSON the serde crate offers a Now if I want to have a struct that combines multiple tables into one representation I need to have a struct for internal use that has a field My proposition is to use the same syntax as serde uses for skipping fields In this example I can use this struct to retrieve it and it's relations and populate the ForeignStruct using the associations API, Shown in the example PagesWithBooks struct ReusableStruct{
#[serde(skip)]
pub id: i32,
#[serde(skip)]
pub foreign_table_id: i32,
#[diesel(skip)]
pub foreign_table: Option<ForeignStruct>,
} Reading this #860 (comment) I do agree that a 1 to 1 representation is better, something like dsync would solve this manual 'duplication' of code but the tool is not configurable enough(yet) to fully automate this unfortunately. An other suggestion I have is to lock this field skipping behind a feature flag to take away the concern that people will create 1+N queries and only enable this when they know what it does and know what they are doing with it. I hope this makes a compelling argument for including this feature. If there are any questions, further clarifications or oversight feel free! |
@AlexStorm1313 Hi! Thanks for the details on your use-case. I think I understand. IIUC the design you currently "have to have" is something like this: #[derive(Queryable, Selectable)]
struct ForInternalUse {
id: i32,
foo: String,
bar_fk: i32,
}
struct UserFacing {
id: i32,
foo: String,
bar: Bar,
} If that is indeed the case, have you considered doing: #[derive(Queryable, Selectable, Serialize)]
struct ForSelect {
#[serde(skip)]
id: i32,
foo: String,
#[serde(skip)]
bar_fk: i32,
}
#[derive(Serialize, derive_more::Deref)]
struct ForUserAndInternalUseAfterPopulatingFks {
#[deref]
#[serde(flatten)]
internal: ForSelect,
bar: Bar,
} AFAICT this also avoids field duplication, while providing you with better typing all along the code chain than your proposed solution (IIUC): #[derive(Serialize, Queryable, Selectable)]
struct ForBothUse {
#[serde(skip)]
id: i32,
foo: String,
#[serde(skip)]
bar_fk: i32,
#[diesel(skip)]
bar: Option<Bar>,
} Typing is better in the previous solution because you have no scenario where you can forget to populate bar and then use it as if it had been populated (that is, by unwrapping "this should have been present", which is verbose in itself, or worse, considering a Vec empty when it should not have been and propagate a further bug). NBs:
|
@Ten0 Thanks for the reply with detailed suggestions. After trying out the suggestions and some variations of it I have come to the same conclusion. The option to ignore/skip fields is not desirable. For anyone that is facing a similar situation my suggestion is to bite the bullet and have separate structs for use with diesel(database). I find that a lot of code can be put in macros and/or traits and be reusable that way. Think of generic CRUD functions or default fields like |
I just ran into this issue with a generated column in postgres which I need the DB to manage, but I have to manually implement 'Insertable' in order to not write to that column. A 'diesel(read_only)' attribute for that column/struct field would have been very helpful. |
@knickish As written several times above: We are open to accept PR's that adds skip attribute for |
Code becomes confusing when having different structures for inserting / updating / querying / .. I think you can use the diesel query syntax to submit only the fields of a struct that you want to insert. Imagine having the following struct:
And you want to insert this Post into the database:
Of couse for large structures this becomes a huge effort to extend and maintain. |
IMO it's even more confusing/dirty when you have to put dummy values and risk misusing fields that contain dummy values. For any post below: |
Hi @weiznich, We need this feature but don't have the skillset to implement it. Would you or someone on the diesel team be willing to accept a monetary sponsorship / bounty to implement this feature? |
@phayes I'm generally open to such sponsorships, but it really depends on what exactly you expect to happen here:
Given that the only attribute that could be sponsored would be |
Hi @weiznich, What I'm looking for is something like in the orignal post that opened this issue, an |
My sentiments are the same, this would be a really good feature to have. |
To repeat what is already written several times above: It's fine that you believe that this would good to have but we as maintainers have not seen a good use case for this that cannot be solved in a better way with existing tooling. Feel free to present a novel use case that is not listed above, otherwise this does not add much to the discussion. |
This whole thread is filled with your combative responses as well as unnecessary policing of comments. It's perfectly reasonable to ask for updates (within moderation, once every few months maybe) because I've seen plenty of issues in FOSS pick up pace with a nudge from the community. Folks have given plenty of use cases for this feature so I'm not sure why you're asking for more. It's probably better for you to work the problem, ask more questions, and listen more to the community than to say "those points have been made already". Having multiple people say the same thing multiple times is actually a good thing. It tells us there is consensus outside of your perspective. |
@marziply To be clear here: I need to do nothing as I own you (any anyone else) nothing. Diesel is free software, that means it is provided as it is without any guarantees. You can use it, patch it, do whatever you want, but you cannot request from me to do something. I might be interested in accepting certain contributions or not. In this case I've already expressed multiple times that we are not interested in this specific feature for the reasons outlined in this thread. I still see no reason to implement that feature as no one cared to explain why the existing solutions are not sufficient and how the outlined problems with the "proposed" solution can be solved. Did you caro to write that down before you claimed that there is "some community" consensus on this feature? I see anything here, but "consensus" as everyone seems to want something with slightly different semantics. So if you really care about this feature I would suggest that you sit down and write a proposal that answers the following questions:
To write that down as last sentence: Please stop to request anything here. This behavior is not welcome as outlined above. |
I've found myself in want of this a few times, usually working around it by having two separate structures, but that inevitably means more code in ways that proc macros are meant to solve. Here are my two cents:
My instinct is to do serde-esque things like skip, skip_serializing, skip_serializing_if, skip_deserializing, default, as, etc. In the case of serde, the field's data type must implement
I don't understand the the internals of diesel that well, but can't the proc macro simply insert |
@1Dragoon I'm sorry but this still repeated the same points that were made before. More importantly it does not address the concerns listed in this (#860 (comment)) and the following comments. It's really not helpful to repeat the same discussion again and again. It also explains the N+1 problem more in depth (Hint: Your proposal does not even touch that problem at all). On a more general note: As some people already claimed that "everyone" agrees on that's a good feature and it needs to be implemented now: There is nothing stopping you from implementing an extension to diesel that supports this feature. After all all the derives are already in an external crate, so you can easily provide an in your opinion enhanced drop in replacement for any derive. Given that no such thing exists today I don't believe that it is something that "everyone" wants. |
Actually I have this struct
The column
author
is not in the database, but when I return the model, I would like to put inside of the struct theAuthor
(a user can or not to have an author). For that I want to tell diesel that ignores the fieldauthor
. I'm searching if something like#[ignore_field]
.The text was updated successfully, but these errors were encountered: