Problema con nuevo encapsulamiento de `NetworkError`.

[tomasmiguez]

Hola, recientemente migramos a la versión 2.x de esta gema, y observamos que empezaron a encapsular los errores de la gema HTTPClient, esto nos está trayendo problemas, ya que dicha gema tiene varios tipos de errores distintos que heredan de TimeoutError (ConnectTimeoutError, SendTimeoutError, ReceiveTimeoutError). Previamente lo que hacíamos era handlear el ConnectTimeoutError y hacer un retry, pero esto ya no es posible ya que no podemos distinguir los distintos tipos de timeouts. No queremos retryear los otros porque puede ser que modifiquen el estado del servicio que consultan. Sería posible cambiar esto? Una opción sería tener distintos errores de esta gema que encapsulen los distintos errores de HTTPClient, seguiría abstrayendo la gema que usan para hacer los requests pero sin perder información. Si les parece una alternativa aceptable podemos ver de implementarlo de nuestro lado. Además esto no rompería la API de esta gema porque los que ahora están handleando NetworkError podrían seguir haciendolo con normalidad, mientras que los que necesiten control mas granular podrían handlear, por ejemplo, ConnectNetworkError (que heredaría de NetworkError).

Gracias!

[eeng]

Hola @tomasmiguez, Gracias por la explicación detallada, tiene sentido.

Como hack rápido para resolver tu problema, la excepción original de HTTPClient debería ser accesible a través del método cause de la instancia de NetworkError, así que de esa forma pueden volver a distinguir entre las distintas subclases.

No me convence mucho mapear los errores de HTTPClient a los nuestros, porque todas las gem alternativas a ésta tiran errores diferentes. Con lo cual, si alguna vez decidimos cambiar de gem HTTP, no creo que podamos mantener la misma API. Por otro lado, sí parece útil que el cliente de esta gem pueda saber si el error es retriable o no, sin tener que referenciar a a la gem que hace los requests por detrás. Y esa característica sí es lo suficientemente general para ser aplicada a todas las gems. Se me ocurren dos opciones:

1. Crear un Afipws::RetriableNetworkError (subclase de NetworkError) para encapsular HTTPClient::ConnectTimeoutError. Los demás errores de HTTPClient podrían seguir mapeados a Afipws::NetworkError.
2. Agregar un método retriable? a Afipws::Error (por defecto en false). Pienso en definirlo allí y no en NetworkError, ya que recuerdo que nosotros reintentábamos también ante ciertos códigos de un ResponseError, por ejemplo. Luego en NetworkError agregamos un constructor que permita setear ese valor (así lo ponemos en true para un ConnectTimeoutError.

Me inclino por la opción 2 ya que me parece más flexible. Qué opinás? En este momento no puedo dedicarle tiempo para implementarlo. Pero si están de acuerdo y dispuestos, adelante por supuesto, ni bien tengas el PR avisame y lo revisamos.

Saludos!

[tomasmiguez]

Dale, tiene sentido lo que mencionas. Voy a intentar implementarlo y paso el PR cuando esté.

Las únicas 2 consideraciones serían que esta interfaz sea medio rara, nunca vi una gema que haga algo parecido. Y la otra que en primera instancia el único error que respondería true sería el de ConnectTimeoutError, así que hasta que se agreguen nuevos habría muchos falsos negativos en ese aspecto, pero si eso no molesta puedo probar de implementarlo.

[eeng]

Perfecto. No creo que sean un problema esos falsos negativos. Siempre podemos ir ampliando la lista a medida que vayamos descubriendo otros casos.