Open metemaddar opened 1 year ago
I agree this would be of added value. Eventually, many endpoints would be affected by this. @metemaddar do you see a sort way we can handle this on a more global level?
Great way. As you said we can handle such errors globally. One way is to use middleware as discussed here: https://stackoverflow.com/a/62407111/1951027
As an example for the layer_library, we have the following exepttion while creating an already exists unique field (name) which raises an sqlalchemy.exc.IntegrityError which has the DETAIL: Key (name)=(test) already exists. We can use the detail to inform client.
self._adapt_connection._handle_exception(error)
File "/usr/local/lib/python3.9/site-packages/sqlalchemy/dialects/postgresql/asyncpg.py", line 663, in _handle_exception
raise translated_error from error
sqlalchemy.exc.IntegrityError: (sqlalchemy.dialects.postgresql.asyncpg.IntegrityError) <class 'asyncpg.exceptions.UniqueViolationError'>: duplicate key value violates unique constraint "layer_library_name_key"
DETAIL: Key (name)=(test) already exists.
[SQL: INSERT INTO customer.layer_library (name, url, legend_urls, special_attribute, access_token, type, map_attribution, date, date_1, max_resolution, min_resolution, source, source_1, style_library_name) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s) RETURNING customer.layer_library.id]
[parameters: ('test', 'https://{a-c}.basemaps.somecdn.com/dark_all/{z}/{x}/{y}.png', None, '{"imagery_set": "Aerial"}', 'some_token', 'BING', 'Attribution to the source', None, None, None, None, None, None, None)]
(Background on this error at: https://sqlalche.me/e/14/gkpj)
I understand! I believe we could test to implement this. It should not break anything from what I know at the moment.
I found a better way of handling errors with the title: Install custom exception handlers. It seems to be the right way of handling errors instead of using middleware and overriding the default error 500 handler. I'm going to test it.
Error was handled through the above commit. But it doesn't provide details about which field to handle. Inside the IntegrityError
exception we have the following data:
# print(exc.__dict__) # exc is IntegrityError
{
"code": "gkpj",
"statement": "INSERT INTO customer.layer_library (name, url, legend_urls, special_attribute, access_token, type, map_attribution, date, date_1, max_resolution, min_resolution, source, source_1, style_library_name) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s) RETURNING customer.layer_library.id",
"params": (
"test",
"https://{a-c}.basemaps.somecdn.com/dark_all/{z}/{x}/{y}.png",
None,
'{"imagery_set": "Aerial"}',
"some_token",
"BING",
"Attribution to the source",
None,
None,
None,
None,
None,
None,
None,
),
"orig": IntegrityError(
"<class 'asyncpg.exceptions.UniqueViolationError'>: duplicate key value violates unique constraint \"layer_library_name_key\"\nDETAIL: Key (name)=(test) already exists."
),
"ismulti": False,
"hide_parameters": False,
"detail": [],
"connection_invalidated": False,
}
I think we need to extract the right column to inform the client about the duplication of unique field.
Added detail to the response message which is straightly printed the orig
part of IntegrityError
. Of course we should extract the right message from orig and have more clear detail on the response side.
At the moment the example response is as following:
{
"message": "object with a unique field already exists.",
"detail": "<class 'asyncpg.exceptions.UniqueViolationError'>: duplicate key value violates unique constraint \"layer_library_name_key\"\nDETAIL: Key (name)=(test) already exists."
}
@metemaddar can we close this one?
Whenever a row with unique field already exists, We need to inform fastapi about the raised error. At the moment, it raises
error:500
For example when already we have a layer_library with name 'test' and we want to create another with the same name, We face error 500 which can be for example
error: 409 (conflict)
and it's detail about the conflicted field.Discussions about error 409: https://stackoverflow.com/a/3826024/1951027