Don't break the effect on exceptions

[domdorn]

Currently, the processApiResponse call throws an exception (when an unexpected response is returned) which will kill the effect handling mechanism without the possibility to recover. This fix should fix that.

[ex0ns]

That looks good, however, could you please create an intermediate variable for readability.

   val extractedBody = monadError.map(response)(_.unsafeBody)
    monadError
      .flatMap(extractedBody)(t => monadError.fromTry(Try(processApiResponse[R](t))))

Thanks !
Also, could you provide a setup in which such an error could happen ? (we don't have tests for this class, but I would like to keep track of this kind of issue for the future)

[domdorn]

I'll try to get one during the week, have a busy day tomorrow and its already late now. The case where this happens is basically anywhere where the telegram api returns an error, e.g. as in my repo, when the bot tries to create an invite link, but has not the right permissions in the room for that, then telegram-api returns a 400.

[ex0ns]

I pushed a first draft for the migration to sttp3 in https://github.com/bot4s/telegram/tree/sttp3
I translated as close as possible from the original implementation and that should also include this fix, however I would love to have a use case I can reproduce on this branch to test that the implementation is correct.

[domdorn]

I pushed a first draft for the migration to sttp3 in https://github.com/bot4s/telegram/tree/sttp3
I translated as close as possible from the original implementation and that should also include this fix, however I would love to have a use case I can reproduce on this branch to test that the implementation is correct.

I'll try to build my new bot against your branch.. could u give me a one-liner on how I can build the jars with mill? I tried yesterday and was not able to do so... or maybe we can publish a snapshot or something.

[ex0ns]

You can publish all the jar locally by using the two following commands:

mill __.publishM2Local  # Maven repo
mill __.publishLocal # Ivy repo

[domdorn]

I'm atm writing tests and noticed, its working correctly with the original code using cats MonadError for Future, so I guess, my implementation of MonadError has a flaw or something. Please don't merge this yet, will keep you posted.

[ex0ns]

I'm pretty interested by this issue as I will try to finish my sttp3 update next week.
I've added a simple test example 4fb380b to demonstrate how to use sttp stub in order to test the STTP client used in bots (I used Future client in this case).

I will try to add more tests in the future, but from my early experiments, it seems that my future is failing with a parsing failure when it can't read the response (400 or 500, the server is not even sending JSON back). And with a TelegramApiException when the server is responding with a JSON that contains an error (Unauthorized for instance).

[domdorn]

@ex0ns I've been in contact with the guys from zio
zio/interop-cats#330
and from what I got, the way to go (e.g. similar to what doobie and others do) should be to transform the def processApiResponse (and in best case every other place that throws exceptions) into something that does not throw an exception but returns a value, even if its just an Either[Throwable, A]

[ex0ns]

This is interesting, we could indeed avoid throwing exception in this part of the code.
I'm pretty confident that the migration should be pretty easy to do as this is a protected method and is only used in a few places (less than 10 actually), would you like to try that ? Otherwise I'll take care of that next week

[domdorn]

I can try to do a refactoring on the relevant parts on the weekend... just wasn't sure on how deep I'm allowed to grab into the code, so I tried with the smallest possible change first..

[ex0ns]

Any news on that ? I think I'll soon merge #110 as it does not seems to be breaking anything.

[domdorn]

Hi @ex0ns!
Sorry, I've been really busy.
Please merge sttp3 at your convenience, I'll (re)base my changes on your branch or the latest master.