dragonflares / tacs-1c-2022-grupo2 Goto Github PK

• Cómo harían para evitar verbos en las rutas /signin y /authenticate y ser RESTful? Piensen el o los recursos que estamos manejando y cambien esas rutas (Commit subido)
• Cuando como usuario logueado creo un torneo, debería yo poder mandar por body quién es el owner? Tiene sentido? Si quiero avisar cuántos puntos hice, por qué tengo que pasar el usuario por body? Podría mandar el username de otro usuario y clavarle cualquier score? :P
• Si como creador quiero agregar un user a un tournament, va bien PUT /tournaments/{id}/participants de acuerdo a REST y HTTP? Entiendo que no está hecha la lógica, pero seguramente haya formas RESTful (y de paso más sencillas) de agregar un user a un tournament. Recuerden que si usan PUT tienen que pasar el listado entero de participantes, evitar el problema del lost update con ETags u otro mecanismo de lockeo, etc. (Commit subido)
• No veo ruta para sumarme a mí mismo a un torneo público que no haya comenzado. Quizás está contemplado el caso y no lo llego a ver
• Falta estructurar las rutas de help y dictionary. Para que lo tengan en cuenta a futuro, si bien no son todos los casos de uso para esta entrega (Commit subido)

• Uso idiomático de Optional: prefieran siempre .map.orElseThrow(...) o .map.orElse(...) y sus variantes con .flatMap antes que .isPresent() y .get(). Pueden empezar acá para ver algunas reglas

• Ya que usan lombok con @DaTa, vayamos por data inmutable y agreguemos private final a todos los dto/model que tengan @DaTa y puedan aprovecharlo. Esto hace que lombok NO genere setters, porque con final el valor final del atributo es el que tiene al instanciarse. Final es un buen default para tener data inmutable y evitarles bugs

• UserDao: sí, User es un mejor nombre para esto. Como lo tenemos en el paquete de modelo, no debería haber problema con ir con este nombre. De paso: DAO es un patrón similar a repository, no es un análogo a DTO para repos (esto vendría a ser un Entity o hibernate POJO) (Commit Subido)

• TournamentConverter.java:convertListTournamentToDto: preferir uso de stream() + map() en vez de nuevo array + forEach para evitar la creación de una nueva lista. tournamentList.stream().map(...)
  (Fix solucionado con el modelMapperTacs)

• TournamentRepository.java: ojo con la API de los repos (o sea, los métodos que exponen). Las interfaces de los repos deben ser agnósticas de la fuente de datos (SQL, noSQL, HTTP, etc) y quedan mejor abstraidos nombres como “create” que “post”

• WordFinder: pueden hacerse un package aparte para servicios de terceros que consuman. Algunas ideas: external -> clients, utils, implementations, integrations, external -> services…

• Optional.of(userRepository.findByName(username).get()); ???

• Ojo que actualmente 2 users nunca son iguales por cómo están comparando users en el registro (las passwords hasheadas nunca coinciden). Al margen, está mal comparar todo el user para ver si me puedo registrar, lo que no se debe repetir es el username (Commit subido)
• Los controllers reciben y devuelven DTOs: el mapeo es para ambos lados. Consideren hacerlo con un mapper mágico/inyectar mappers (Commit subido)
• Si van a explotar ante ciertos escenarios (JwtAuthenticationController.java:59) tengan en mente implementar un @ControllerAdvice que handlee cómo responder ante cada error (o implementen varios handlers, el Advice es un consejo). La idea es centralizar el manejo de excepciones en un lugar claro (y no exponer el server con un 500 si reciben un null… devuelvan un 400!)
• Siguiendo con esta idea, formalicen el manejo de error usando excepciones de negocio o validación según crea conveniente, y ayúdense con el ControllerAdvice para catchearlas y ahí tirar el error correspondiente con su status code 4xx o 5xx
• Siguiendo esto, en UserRepositoryInMemory.java:31 tiran una excepción que hace explotar con 500. Conclusión: excepciones de dominio + manejo de status codes va a mejorar mucho su API
• Para casos simples (ej: body con atributos no nulos) usen @Valid de Spring. Aprovechen que tienen DTOs para definirles dentro las reglas de cada atributo

Deseable:

• GET de todos los torneos públicos: acá tiene que haber paginación y algún order, no podemos devolver todos los torneos de la historia. Vayan por soluciones documentadas y battle-tested en estas cosas
• Dos puntos acerca de ResponseEntity.ok(new JSONWrapper<>...:

1. Usando Spring no hace falta indicar el ResponseEntity.ok en cada endpoint, Spring se encarga de hacer esto automágicamente. Si desean usar otro status code de éxito, simplemente se lo indican con @ResponseStatus u otro mecanismo (Commit subido)
2. JSONWrapper: primero: con los éxitos no se acostumbra enviar “message”, sino que en los wrappers de error se usa para hacer responses como { message, cause, error } o similar. Les sirve de algo meter “message” en casos OK?
   Segundo: si evalúan que van a usar JSONWrapper en todas las responses de éxito, no les conviene generalizarlo en vez de repetirlo? Pueden implementarlo para ahorrarse bastante código. Les tiro una pista si quieren implementarlo, para que no se maten tanto: un @RestControllerAdvice que extienda AbstractMappingJacksonResponseBodyAdvice :) (Commit subido)

• JWT: si ven que realmente no les aporta nada custom, pueden elegir esta implementación que es más sencilla (menos código) que la que siguieron ustedes. No es lo más importante a esta altura, pero estaría bueno entender el flujo entero de JWT, handlear contra todos los errores que pueda tener, etc
  -- JWT, corrección para entregas 4+:. el TP pide logout, pero acá no tenemos logout (los JWT son stateless, hacerles logout no tiene sentido). Si hiciéramos manejo stateful de sesiones, sí tendríamos logout. Entiendo que JWT es mucho más simple de implementar y más escalable. Lo clásico es que el cliente (front) borre el JWT de su storage al hacer clic en “logout” (entrega 3). Esto borra el JWT del cliente, pero si alguien roba ese JWT puede seguir logueado hasta que expire y nosotros perdemos el control. Cómo controlamos desde la API un posible robo de JWT? Imaginen que un cliente denuncia que le robaron las credenciales y hay que anularlas rápido. Podemos hablar cómo mitigar esto de cara a las últimas entregas (Comentario subido)

• Su Dockerfile incluye solo la parte de run. Si bien esto es muy útil para la corrección, para la próxima entrega les pido que usen multi-stage builds de Docker y que incluyan en el Dockerfile la parte de compilar (grade build), así su app puede buildearse y con un segundo FROM pickean el jar y lo ejecutan (esta última parte ya la están haciendo). Con la guía que ya usaron y gradlew debería alcanzar por ahora (Commit subido)
• Empiecen a usar javadoc. Si bien por ahora no han implementado la parte pesada de la lógica, va a ser necesario
• El primer test de su app es el test mockeado de un controller. Si bien todavía no tienen implementada la lógica, recuerden empezar por tests unitarios
• El test que tienen TournamentControllerTest no está mal, pero recuerden pensar qué testeamos cuando testeamos. Por lo que veo están testeando un contrato (que el response sea un JSON deserializable, que el response sea 200 OK con ciertos headers, etc) lo cual está bien para prevenir deployar algo que cambie el contrato de su API. De paso hardcodeen más su objectMapper para que refleje lo que quieren fijar en su test como contrato (por ejemplo, que use camelCase sí o sí)
• Consejos sobre testing:
  estructura de carpeta /test: usen los packages para ventaja de ustedes. Si imitan la estructura de su carpeta /src, no van a necesitar importar nada propio del test que estén haciendo (Commit subido)
  tengan presente usar fixtures/sample objects (singleton de test) para los test unitarios si piensan que les conviene