catch unique constraint violations when creating users

[iasoon]

Example code for catching unique constraint violations directly from database-side.
Related to #177

Not sure whether this is the way we'd want to go forward with this, but I thought I could create a working example to get the ball rolling on this issue :)

A downside to this approach is that when two constraints are violated simultaneously (eg. both username and email are taken, only one constraint violation will be reported.

[rien]

Handling the error does seem simple enough. I've extracted the error handling to a separate function to simplify it even further.

We could also add the error handling to src/errors.rs, but I think we want to keep the constraint error handling as explicit as possible.

zauth/src/errors.rs

Lines 184 to 191 in df6af32

	impl From<diesel::result::Error> for ZauthError {
	fn from(error: diesel::result::Error) -> Self {
	match error {
	NotFound => ZauthError::not_found(&error.to_string()),
	_ => ZauthError::Internal(InternalError::DatabaseError(error)),
	}
	}
	}

[rien]

I have added some tests and this seems to be working. I think the current code is simple and reusable enough to change my opinion on #177.

Any more opinions, @Tibo-Ulens, @iasoon?

[Tibo-Ulens]

It's a bit unfortunate that we have to match against raw string values in the error (let's hope diesel doesn't change those in a random update) but this does seem like a good solution.
I think it might be possible to create a custom validator for the new handle_constraint_error function as well, seeing as validators can accept extra arguments so it might be worthwhile to do that as well, as this would mean the map_err statement can just be absorbed by the call to user.validate()?.

[iasoon]

@Tibo-Ulens These string values don't come from diesel, they come directly from the constraint name in postgres. We control these, and I'd argue that this is the same as hardcoding the database table names corresponding to the different models.
From the validator docs, I think it should certainly be possible to do the check in validations as well.

I think this approach is technically better(tm) as it avoids additional database round-trips, and does not suffer from phantom write issues (when another user registered the username between your uniqueness check and final insert query).
In context though, these considerations seem irrelevant - with the expected traffic levels phantoms would be extremely unlikely, but even when they would happen, an user would just have to re-submit his registration and all would be well.

Adding these checks to the validations would be nice, since all validation logic would be in one place. On the other hand, this would require having the database available for running validations. That means the validation logic will be stateful (which is more cumbersome for eg. testing). When you'd separate the stateful and stateless elements, I think you'd essentially end up with an approach equivalent to what's implemented here.
So in the end I think it comes down to opinions :)

That said, though: I think a nice side side-effect of the current approach is that we disambiguate between invalid requests (username is too short or contains disallowed characters - the request does not obey the spec and thus it is unprocessable), and valid requests that could not be processed due to the system state (such as registering an username that is taken, for which 409 might be a more appropriate status code).

[Tibo-Ulens]

@iasoon i fiddled around a bit with trying to write a validator function like this in a project of my own and it turned out to actually be quite annoying to get it to work as a generic function (otherwise you'd have to write a function for every field to check for uniqueness which is very annoying). So considering that, i agree that this is probably the best approach for now, and it has the benefit of being atomic as well in case the entire faculty decided all at once to make a zauth account.

[rien]

I think we have come to the conclusion that this is a good fix for #177.