Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow arbitrary non-Siesta promises in request chains #152

Closed
Tazer opened this issue Nov 27, 2016 · 8 comments · Fixed by #254
Closed

Allow arbitrary non-Siesta promises in request chains #152

Tazer opened this issue Nov 27, 2016 · 8 comments · Fixed by #254
Assignees

Comments

@Tazer
Copy link

Tazer commented Nov 27, 2016

Hello,

I'm trying to implement this example: http://bustoutsolutions.github.io/siesta/guide/configuration/#decorating-requests-via-configuration

But getting into trouble , cause I'm getting the new token async, so don't see how I should make that code work. This is how far I have gotten:

    func refreshTokenOnAuthFailure(request: Request) -> Request {
        return request.chained {
            guard case .failure(let error) = $0.response,  // Did request fail…
                error.httpStatusCode == 401 else {           // …because of expired token?
                    return .useThisResponse                    // If not, use the response we got.
            }
            
            
            
            
            if let refreshToken = self.authService.getRefreshToken(){
                self.authService.fetchNewToken(refreshToken:refreshToken).then { token -> Void in
                     //Async stuff , how should I do to retry the request here ? <-- problem here
                    // callback request.repeated()
                    }.catch {error in
                        //also .useThisResponse.
                }
            }
            
            
            
            return .useThisResponse;
        }
    }

So this code does kinda nothing. I have looked at the closest issues also but don't really see an solution to my problem.

Anyway that could guide me in the right direction ? :)

Just a FYI: I'm kinda new to Swift/iOS dev, so maybe I'm missing something obvious.

@Tazer
Copy link
Author

Tazer commented Nov 27, 2016

Manage to solve it I think.

Just running resource.load()when I got the token 👍

So basically just letting the request fail then reload stuff when I have the new token.
It will work fine for GET requests at least. and that's fine for now.

@Tazer Tazer closed this as completed Nov 27, 2016
@pcantrell
Copy link
Member

Just running resource.load()when I got the token 👍
So basically just letting the request fail then reload stuff when I have the new token.

Yes, that’s the way to do it if you want resource observers to first see “no auth” failure, then success:

  1. load() → observers see loading
  2. load fails → observers see “no auth” error
  3. auth succeeds; load() again → observers see loading
  4. load succeeds → observers see new data

The request decorator is what you do if you want the whole auth reattempt to look like a single request from the outside, so that observers / requesters to just wait until the auth attempt succeeds or fails:

  1. load() → observers see loading
  2. load fails, but observers still waiting
  3. auth succeeds, so reattempt request; observers still waiting
  4. load succeeds → observers see new data

In your example code, where do fetchNewToken(…) and .then come from? Is that another library?

@Tazer
Copy link
Author

Tazer commented Nov 28, 2016

@pcantrell Thanks for your answer and I'm with you , guess it doesn't "matter" that much in my case. But it would be more performant to do the "second" flow. But not sure how to solve that.

Yes.

func fetchNewToken(refreshToken : String) -> Promise<String?> {
        return Promise { fulfill, reject in
            A0Lock.shared().apiClient().fetchNewIdToken(
                withRefreshToken: refreshToken,
                parameters: nil,
                success: { token in
                    
                    self.setToken(token: token.idToken);
                    if let newRefreshToken = token.refreshToken{
                        self.setRefreshToken(refreshToken: newRefreshToken)
                    }
                    fulfill(token.idToken);
            },
                failure: { error in
                    log.error(error);
                    reject(error);
            });
        }
    }

So fetchNewToken is using Lock Auth0's iOS client https://github.com/auth0/Lock.iOS-OSX and then I'm using PromiseKit , but I would get into same problems without PromoiseKit cause promisekit is just a helper library, so I don't need to pass around callbacks everywhere :)

So the problems starts with A0Lock.shared().apiClient().fetchNewIdToken() cause it's async and takes a success callback.

Would you have any suggestions how to integrate that with the request decorator ? :)

@Tazer Tazer reopened this Nov 28, 2016
@pcantrell
Copy link
Member

pcantrell commented Nov 30, 2016

Yup, then there is an issue to resolve here. Siesta uses RequestChainAction instead of more open-ended promises to ensure at compile time that chained requests honor the general Request contract of having exactly one result.

That’s lovely if you’re staying entirely inside the world of Siesta requests, but it gives you no simple way to wait on something else (such as a PromiseKit promise) mid-chain. It’s possible to do it by writing your own Request implementation that wraps a promise, but Siesta ought to provide that escape hatch for you. I’ll leave this issue open for that.

@pcantrell pcantrell changed the title How to: decorateRequests with callback methods Allow arbitrary non-Siesta promises in request chains Nov 30, 2016
@pcantrell pcantrell self-assigned this Jan 8, 2018
@AkdM
Copy link

AkdM commented Jan 31, 2018

Any update on this, Paul? 🙂
Having something like interceptors, where you can intercept requests or responses before they are handled would be good, with promise/async support.

pcantrell added a commit that referenced this issue Mar 17, 2018
@pcantrell
Copy link
Member

pcantrell commented Mar 17, 2018

You can find mostly working implementation in progress on the simpler-custom-requests branch. I’d love for you to poke at it a little and share your thoughts!

There’s no documentation yet, not even API docs, so it may be a bumpy ride. Here are some quick tips to get started:

  • The new additions to the public API are here.
  • Create an implementation of RequestDelegate to wrap a promise, OAuth lib, etc.
    • The crucial method to implement is startUnderlyingOperation(completionHandler:).
    • Ensure that your implementation eventually calls completionHandler.broadcastResponse(…).
    • Note that this does not touch the response pipeline: no transformers, no cache. Whatever you pass to broadcastResponse goes straight to the request’s callbacks, so it’s up to your delegate to put the response in whatever form the rest of your app expects. The assumption here is that whatever external library you’re using does its own processing, and does not just return raw Data, so normal pipeline operations should not apply.
  • Use resource.request(using: myCustomDelegate) to initiate a Siesta request using your delegate. It returns a Request that you can use in request chains, pass to resource.load(…), etc.

If you get a chance to give it a try, please let me know! I would love to get feedback before publishing the new API.

pcantrell added a commit that referenced this issue Mar 17, 2018
pcantrell added a commit that referenced this issue Jun 4, 2018
@jordanpwood
Copy link

jordanpwood commented Nov 14, 2018

@pcantrell, I noticed this feature in the release notes, and I'm really glad its there! I am trying to add a custom RequestDelegate and put it in a chain. I have written a test to check that I am using this feature correctly, but I can't get the RequestCompletionHandler.willIgnore to not return true. It looks like the LiveRequest that Resource.prepareRequest(using:) is returning is expecting onCompletion to be called on it by something. My test has a very simple RequestDelegate that doesn't wait, and calls willIgnore and broadcastResponse from within startUnderlyingOperation(passingResponseTo:). Is this synchronous behavior the problem?

Also, my real implementation that uses this feature will open a web view and get the user to login so I can get the credentials I need for Siesta's headers. I assume that this non NSURL based request will work in a request chain?

@pcantrell
Copy link
Member

I can't get the RequestCompletionHandler.willIgnore to not return true.

This means that the Request thinks it has already either received a response or been cancelled. It’s hard to say why this is happening, but it would start with examining whether you’ve already made a call to broadcastResponse before calling willIgnore.

Synchronously calling completionHandler within startUnderlyingOperation shouldn't be a problem. I verified that this works as I’d expect it to:

Resource.prepareRequest(using: DummyRequestDelegate()).start()


class DummyRequestDelegate: RequestDelegate
    {
    func startUnderlyingOperation(passingResponseTo completionHandler: RequestCompletionHandler)
        {
        let dummyResponse =
            ResponseInfo(
                response: .success(Entity<Any>(
                    content: "custom",
                    contentType: "text/whatever")))

        print(completionHandler.willIgnore(dummyResponse))  // prints false
        completionHandler.broadcastResponse(dummyResponse)
        print(completionHandler.willIgnore(dummyResponse))  // prints true
        }

    func cancelUnderlyingOperation()
        { }

    func repeated() -> RequestDelegate
        { fatalError("unsupported") }

    let requestDescription = "DummyRequestDelegate"
    }

Also, my real implementation that uses this feature will open a web view and get the user to login so I can get the credentials I need for Siesta's headers. I assume that this non NSURL based request will work in a request chain?

Yes, that should work fine, and is a good use of the feature. A more standard approach would be to accept the error and prompt a retry — but if you want parts of the UI to just sit there waiting until the user logs in, then what you describe is exactly the way to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants