zenhack
commented
6 years ago We should definitely add a unique constraint everywhere it makes sense.
Re: catching the error from SQLAlchemy, I'm not sure it's even the same across database backends. I seem to remember the postgres backend throwing a ProgrammingError or somesuch in an analogous situation.
Also, I don't want to be over-broad in what errors we're catching -- blindly assuming any IntegrityError is because of the unique constraint violation could end up masking real bugs.
The way I've had this in my head (and if we agree on it we should probably write it down as an official guideline) is to basically write code that should never cause SQLAlchemy to raise an exception. Doing this has the nice advantage that if we get a stack trace with an IntegrityError in it, we don't have to waste time figuring out whether or not that's supposed to happen.
Finally, I don't think just catching the sqlalchemy exception would even be any more concise; we still need to translate it into an APIError to report it to the client, and _assert_absent/_must_find aren't a lot of code to begin with.
naved001
xuhang57
We use functions like _assert_absent to enforce this in the API.
I think we should put this constraint on the database itself, because one could, when writing tests, directly add objects to the database and could lead to mistakes if the devs aren't careful enough. Example: This method in tests for the networking server adds a new node with the same name every time a new nic is created. This made it kinda difficult to debug stuff.
Once we do that, we could just check SQLAlchemy's integrityerror rather than using _assert_absent. I don't know why we do this to begin with, is there a good reason?
@zenhack thoughts?