Fix converter getting called twice if using an authenticator with a JsonConverter on the request

[maxroehrl]

Fix _handleRequestConverter getting called twice if using an authenticator with a JsonConverter on the request.

Otherwise an already encoded body string in req.body will be encoded again after the authentication challenge was solved.

So in the end in this MR we call send() with the same arguments as the initial request which fails with 403 as the token is outdated.

This behaviour was last changed in #310 to fix #308.

[JEuler]

But it will return the #308 error, as I understand

[maxroehrl]

Another solution would be that we add the original/not converted request as a new parameter to authenticate().

So in my own code I can return the right request and the bug in #308 is not reintroduced.

[JEuler]

Whoops, @maxroehrl could you please dart format the code in Changed lib/src/authenticator.dart? :)

[maxroehrl]

@JEuler I have formatted the file.

[JEuler]

@stewemetal Hi! I think it is okay, just one more param to allow customization. What do you think?

[stewemetal]

It's OK in terms of solving the issue, but it's kind of not OK as this would be a breaking change on the public interface of the lib.

Whoever uses authenticate already will get a missing param error after upgrading to the latest version.

I'll try to come up with something for this today evening, that would break compatibility gracefully, or not at all.

However, this would definitely mean a minor version update either way as this is a breaking change, and it needs to be documented at least in the update note.

[JEuler]

Maybe we can go with optional then? :)

[JEuler]

Whoops, I misunderstood the problem. :) But anyway, it is a light change for client code, so, maybe it is not very BIG. Thank you!

[JEuler]

@stewemetal Hi! Any news? :)

[stewemetal]

Yeah, we can go on with having it as optional I guess. :)

[JEuler]

The option will also break the interface (meaning, the client will need to update the signature in his Auth class). But maybe it is okay. :)

[JEuler]

@maxroehrl Could you please make this param optional? Like [Request? originalRequest]

[maxroehrl]

Yes, the users will still need to update their method signature, but I see no way to prevent this.

[JEuler]

Yeah, I think it is okay. Sorry for the delay, a hard-working week. I will merge it and create new PR with version update : )

[JEuler]

And we are passing the original request inside so the optional is nothing here... but anyway, let's go :)

[flodaniel]

I just updated the package and worked around the bug, this PR is solving.
@JEuler is it correct that the new property originalRequest can never be optional? Why make it optional then?

[JEuler]

Correct, as I told it in "And we are passing the original request inside so the optional is nothing here... but anyway, let's go :)"
Will fix this glitch later, not important.

[flodaniel]

Sorry about that, I was not quite sure about the phrase - english is not my primary language :)

Awesome! Thanks to everyone here - this allows me to remove my dirty fix 👍

[JEuler]

Thank you! Hooray! 🎉