Cleanup: remove email field on User in Rust code

[carols10cents]

This is for someone who likes fixing compiler errors and knows or wants to learn about diesel :)

As of this PR, I'm pretty sure we don't use the email field on User anywhere directly anymore (and if we aren't, we shouldn't be). Instead, we have an emails table that joins to users.

So we can clean this field up and be less confusing about where the user's email is.

The first step is removing all usage of this field from the Rust code, but leaving the column on the users table in the database. Once the Rust code is no longer using the column, we can safely migrate the database to drop that column.

But this issue is for just the first step. The way I'd go about it, which isn't the only way so feel free to go about this the way you'd like, is to remove this line from the User struct definition and try to compile.

A lot of the error messages are going to be because the default behavior of diesel is to select all columns of a table and expect the struct definition to match. Removing the email field from the struct will break that assumption. We need to change the Rust code to never select the email field from the database. We have this problem with krate too, which has a column having to do with full text search that we never want to select in the Rust code.

The way the krate code works is it defines a type alias and a constant named ALL_COLUMNS:

crates.io/src/models/krate.rs

Lines 50 to 76 in 0a6d3fb

	/// We literally never want to select `textsearchable_index_col`
	/// so we provide this type and constant to pass to `.select`
	type AllColumns = (
	crates::id,
	crates::name,
	crates::updated_at,
	crates::created_at,
	crates::downloads,
	crates::description,
	crates::homepage,
	crates::documentation,
	crates::repository,
	crates::max_upload_size,
	);
	pub const ALL_COLUMNS: AllColumns = (
	crates::id,
	crates::name,
	crates::updated_at,
	crates::created_at,
	crates::downloads,
	crates::description,
	crates::homepage,
	crates::documentation,
	crates::repository,
	crates::max_upload_size,
	);

And then everywhere that we select a krate from the database, use that constant instead. For example, selecting all crates happens with the all function:

crates.io/src/models/krate.rs

Lines 245 to 247 in 0a6d3fb

	pub fn all() -> All {
	crates::table.select(ALL_COLUMNS)
	}

which uses the ALL_COLUMNS constant and another type alias named All:

crates.io/src/models/krate.rs

Line 81 in 0a6d3fb

type All = diesel::dsl::Select<crates::table, AllColumns>;

Other convenience functions like Crate::by_name:

crates.io/src/models/krate.rs

Lines 237 to 239 in 0a6d3fb

	pub fn by_name(name: &str) -> ByName<'_> {
	Crate::all().filter(Self::with_name(name))
	}

chain filter calls off all to avoid having to duplicate the .select(ALL_COLUMNS) bit.

Extracting some helper functions might be an intermediate step that can be done in a separate commit from, but informed by, the fixes to the compilation errors. In other words, the errors can tell you where we're currently selecting users from the database directly that we could maybe be using a helper function instead. Extracting those could be done in a commit that passes tests as it would be a smaller refactoring that wouldn't change behavior. Then the commit that removes the field from the struct would need fewer changes in fewer places, hopefully. This may not actually be the case, or may not actually make the changes easier, and it may be just the way I work. If this paragraph doesn't make sense, ignore it!

[hbina]

I would like to work on this!

[carols10cents]

@hbina great!! Please let me know if you have any questions!!!

[hbina]

So if I am reading your suggestions correctly, the idea is to:

1. take note of where this struct is used by diesel,
2. see if any of these usages are using the email variable,
3. if so, try to refactor out this usage by using a helper function that use some other ways to obtain User's email (?)
4. this will hopefully pave way for future plan to drop email column from the table

Correct? From cargo check, there are 19 places where I need to check...

[carols10cents]

4. this will hopefully pave way for future plan to drop `email` column from the table

Correct?

You've got it!

This part looks like an authentication thingy. I don't quite understand how it all works but it does not seem to rely on the email variable at all? The only variable that matters here seems to be the id.

Correct! The error message is complaining about what the database is returning, that the database results don't match what diesel was expecting for the User struct. This spot doesn't make use of user.email at all. I expect most of the errors to be like this.

[hbina]

So, I think the only non-trivial cases are error number 9, 15, and 17. The rest are just simply Diesel error that should be fixed when the table schema are updated. I will copy them here for ease.

[hbina]

If we are ditching email in User, what about this?

crates.io/src/views.rs

Lines 161 to 170 in 3248b4d

	pub struct EncodablePrivateUser {
	pub id: i32,
	pub login: String,
	pub email: Option<String>,
	pub email_verified: bool,
	pub email_verification_sent: bool,
	pub name: Option<String>,
	pub avatar: Option<String>,
	pub url: Option<String>,
	}

There are a bunch of tests that will need to be revised when we do make the change.

@carols10cents

[carols10cents]

If we are ditching email in User, what about this?

crates.io/src/views.rs

Lines 161 to 170 in 3248b4d

	pub struct EncodablePrivateUser {
	pub id: i32,
	pub login: String,
	pub email: Option<String>,
	pub email_verified: bool,
	pub email_verification_sent: bool,
	pub name: Option<String>,
	pub avatar: Option<String>,
	pub url: Option<String>,
	}

We'll need to pass the email into the encodable_private method. @hogum has implemented the actual removal of the column from the database, which can be deployed after this PR gets deployed-- parts of their PR might be relevant here (but the actual dropping of the column shouldn't be moved into this PR)

Let me know if that doesn't answer your questions!

[carols10cents]

Hi @hbina, I saw that you had some questions in chat, I'm going to try to answer your questions here :)

This issue is important because we need the Rust code to stop using the email column first, and we need to deploy that change, so that the servers restart and stop expecting the column to be there before we actually change the database. If we didn't do this, when we do the migration to remove the column, the servers running will have their expectations of the database schema pulled out from under them and that could cause errors.

It's hard to see, but by default, Diesel selects all the columns from a table. For this issue, we need to tell it to select all the columns from the users table except for the email column.

So there will be some temporary code changes needed for this issue that will be undone by #1891, and that's exactly correct. Does that make sense?

I see this commit you posted in chat, it's close and I'm going to add some comments on it!

Please ask more questions if I'm still not being clear!

[hbina]

Hi! Thanks for the feedback. I have applied the changes that you requested. Is this sufficient? I think I am getting the hang of it. I will try to work on it tonight after school, thank you!

[thiagoarrais]

Should this be closed?

[mugoh]

This seems to have been solved by #1911 and #1891 if I remember correctly @thiagoarrais

[JohnTitor]

Going to close this in favor of the above comments. Thanks for the heads-up!