retry: Allow changing of initial request parameters

[Kraigie]

I think it would be useful to have some pathway to modify the initial request before retrying in cases where failure is expected and can be rectified.

Say you make a request to an endpoint with an expired token. This would allow us to request a new token before retrying. It's totally possible that this functionality should live outside of the existing "step" functionality offered by this library, but figured I'd throw it up for discussion. It's also possible this functionality exists elsewhere in which case please let me know as I'm pretty new to the project!

I imagined it to look something like this

Req.get("https://www.example.com/some/private/resource",
  auth: {:bearer, "foo"},
  retry: fn request, _response ->
    {true, Req.Request.put_header(request, "authorization", "Bearer bar")}
  end
)

Thoughts?

[wojtekmach]

Thanks for bringing this up.

I'm curious about more use cases around this. If it's mostly around auth, I was thinking about something like this for a while anyway:

auth: {:bearer, fn -> "foo" end}

would that help?

[Kraigie]

It's not super clear to me how that's functionally different from the existing behavior unless that auth step you've proposed is run when encountering an error, taking in a function that returns a new token. If that's how I'm supposed to read it then I think that makes a lot of sense and would solve my issue!

I'm struggling to think of a use case for my initial issue besides auth, so I think you're right that it should probably live in that space. Thinking on it, if we went with my initial suggestion we'd likely want to include what retry attempt we were on as well, and at that point I think it's pretty messy.

[wojtekmach]

The difference would be every time we make a request (eg retry) we’d call a function to grab the auth as opposed to using the same value.

FWIW we can do that today:

Req.new()
|> Req.Request.prepend_request_steps(custom_auth: fn r ->
  Req.update(r, auth: {:bearer, token})
  # or set the header directly here instead
end)

Thinking about it some more, I hope above is good enough. I’m gonna keep the issue open for a while in case some more ideas appear.

Regarding retry count, you can grab it from req.private. I plan to document it so it can be relied upon going forward.

[Kraigie]

Thanks for the clarification!

Upon further review I think this is sufficient. Most APIs will return how long the token is good for and we can use that to determine if we need to fetch a new token before making a request. This should be able to be handled how you proposed!

My initial suggestion was a much lazier way of doing this. I'm not sure there is merit including it as its probably not within best practices. Maybe there are other use cases for my proposed functionality but I can't think of any. Totally fine with leaving this open to hear from others!

[thenrio]

Hi there, I noticed that retry does not run the request_steps.
In https://github.com/wojtekmach/req/blob/main/test/req/steps_test.exs#L1587, the params step is not run on retry.

So when I do something like

r = Req.new(retry: &safe_transient_or_401/2, retry_delay: fn _n -> 5000 end)
|> Req.Request.prepend_request_steps(ensure_valid_token: fn r -> 
  # use token if valid or fetch a new one, then set header authorization 
end)

With the intent to retry also on 401,
Let token x be valid for 2s,
When I run r and I get a 503
Then I wait for 5s, and I retry
Then I get a 401, because I still send token x, because ensure_valid_token did not run
Then I wait for 5s, and I retry
Then I get again a 401, because I still send token x, because ensure_valid_token did not run.

WDYT @wojtekmach?

[herissondev]

I'm having the exact same issue as @thenrio. I have a step to initialize special headers and a response step that retries the request with some new header values (stored in req.private), but the request_steps is never reran, and thus the request always fails.
Is this the intended behavior?
Anyway, thanks for the great lib!

[wojtekmach]

Yes, this is a known issue and in this particular case it is definitely undesirable. See #255. I don't yet know how to address it but I hope to address this soon.