search

juanmbellini / PowerUp

Web Applications Course Project
0 stars 1 forks source link

Centralize error management #28

Closed juanmbellini closed 6 years ago

juanmbellini commented 7 years ago

Errors that occur in the app are mostly managed by JAX-RS ExceptionMapper, but if errors occur inside the springSecurityFilterChain they are not managed with them. Some are manually managed, but there is code repetition. Consider adding some sort of object that is in charge of managing errors, which can be called from jersey and from spring security.

lipusal commented 7 years ago

Podés linkear ejemplos donde haya código repetido, donde se podría poner un objeto, etc? Por si no sabés, alcanza con ir a la tab Code arriba, cambiar a dev y apretar T, con eso podés buscar archivos. Si en un archivo clickeás en un número de línea te cambia la URL para linkear directamente a esa línea. Por ejemplo, acá está el OPTIONS para login.

juanmbellini commented 7 years ago

En realidad no repetimos el código, pero porque (creo yo) no está bien resuelto el tema. Por ejemplo, acá se está lanzando una excepción que se termina handleando acá (aunque no en un try-catch, sino que asumiendo que si no existe el atributo loginRequest en la request, es porque el JSON que te mandaron estaba mal), sin avisar el motivo del error (tenemos una sublcase de ApiErrorDto que sirve para dar mensajes al consumidor de la API acerca de qué es lo que estuvo mal en la request).

Lo que yo propongo es extraer la lógica que existe en el paquete ar.edu.itba.paw.webapp.exception_mappers(ver acá) en algun Beanal que se le pase la excepción (en este caso sería alguno de los subtipos de JsonProcessingException), y te devuelva algún objeto container con el response status code y response body (el ApiErrorDto especifico para este error).

De esta manera quedaría más consistente la API además.

lipusal commented 7 years ago

Conceptualmente me parece buena idea, pero no sé qué tan fácil sea implementar eso y cuánto nos cambie el código. Si es hacer otro mega refactor sólo para resolver ese tema me parece que no vale la pena. Recordá que no estamos haciendo una API pública, la API es para que la use la webapp y de nada le va a servir que la webapp le diga al usuario "Te faltó una } en la request", se supone que el JS sabe manejarse con la API y que los únicos errores que le deberían llegar al usuario son de credenciales inválidas. Cualquier otro tipo de error al usuario no le incumbe, es un "Server error, please try again."

juanmbellini commented 6 years ago

@lipusal Me parece que cerramos esto, no?