Moya: Improve Endpoint Documentation

[ashfurrow]

Failure is truly a network failure – the status code is optional, even. It's meant to simulate when the device cannot connect to the internet. The Success case is a "success" in that the API returns some data. it's agnostic to the idea that a 404 would be a failure – it succeeded because it returned data.

[skywinder]

+1. Fully agree with you. It's quite confusing to see success, when actually there is no relevant data in json.

[skywinder]

About response codes, my thoughts:

• Response 2xx - successful operation.
• Response 3xx says that for the successful execution the request is necessary to take additional action. - it's not an error. _(luckily Alamofire handle automatically most of this responses, so we don't need to worry about that)_
• Response 4xx says that the client, making the request, did something wrong. - this is error behaviour
• Response 5xx indicates that the client did everything right - a problem on the server side. - it's discussible but I think it should be also error behaviour

[ashfurrow]

True, but there are also error conditions in The iOS network stack that we want to simulate. Maybe we should rename these two cases then.

[damianesteban]

Concerning the endpoint documentation:

I'm working on an iOS application that fetches data from the GitHub API and I've already OAuth authentication. Originally I was planning on using Alamofire to make the authenticated requests to GitHub, but I've been wanting to use Moya for a while in a larger project. Anyway, I was going over the documentation on endpoints and I saw the OAuth example but it is not very clear to me how to implement it in an actual application. Perhaps the Artsy application would be a good place to look?

[ashfurrow]

That's a good idea, @damianesteban. I don't believe we actually use OAuth; we only need to add an HTTP header.

I feel generally that Moya is spreading in a few different directions, and we don't have a set of best practices that demonstrate or implement common practices, like OAuth. I don't think we need to implement MoyaOAuth or anything :grimacing: but some examples of using common libraries would be very useful, too.

[ashfurrow]

So, thanks for the suggestion!

[damianesteban]

You are welcome. Yes, I completely agree that Moya doesn't need a separate OAuth component to it :smile: . I have OAuth up and running with good old NSURLSession and I plan on eventually moving over to Moya once I get the rest of the requests a but more fleshed out. Thanks!

[ashfurrow]

Awesome! Let us know how it goes!

[damianesteban]

I definitely will.

[Jeehut]

It just happened two days ago that one of my colleagues took over a project I implemented a new API class for using Moya and he was completely confused by the fact that an API error (regardless of 4xx or 5xx) was returned within the .Success case. Although I understand and like the fact that Moya only forwards network errors to the .Failure case what about renaming the generic .Success and .Failure cases to something more clear? For example simply adding Network as a prefix resulting to .NetworkSuccess and .NetworkFailure should already improve the clarity a lot. What do you think?

[ashfurrow]

@Dschee it's a good question, and it's one that we've faced before. I'm up for the change, but it would mean abandoning the Result dependency and the nice functions that accompany it like flatMap. It's too bad we can't typealias specific cases of an enum.

[Jeehut]

Hmm, I understand now why those are named the way they are.

But the good news is that the Result dependency has exactly the same license as Moya does (MIT). :) Therefore what about we create our own fork with only the renamed cases and use that? Should be a straight forward find & replace. And in case we want to update the dependency rebasing shouldn't be hard either. Also if I remember correctly I read about plans for the Swift Package Manager to support dependencies with alterations, so maybe we can even drop maintaining the fork sometime?

Not very happy about the suggestion myself, but that would definitely solve the main problem.

[ashfurrow]

Yeah, we definitely can (I'm on the Result team) but I'm not sure it's a great idea, as you mention. Then again, I don't see any alternatives (I'm not a fan of documenting our way out of this – basically just blaming the user). Let's sleep on it and see if we think of any alternatives?

[Jeehut]

Sure, no hurry on my side.

I think we have two more options:
First we could of course try to convince the Swift community that we need support for typealias for enum cases in Swift 3. Second option is we create a wrapper around Result that simply passes through all features we need for Moya from the Result project.

The first option (if it would be successful which we can't be sure of) would delay the support for a better naming until at least September so I think we should only discuss the second alternative for now. What do you think about a wrapper?

[jasl]

Result seems to be a pattern now, actually Alamofire has its own modified version of Result, see https://github.com/Alamofire/Alamofire/blob/master/Source/Result.swift

So, maybe improving Moya.Error is a better way, to clear what failure is, currently, Moya will wrap all Alamofire's error (which is NSError) to Error.Underlying(error), and NSError not easy to do pattern-match.

Consider that if we add more error types, such as Timeout, Cancelled

provider.request(.UserProfile) { result in
  switch result {
  case let .Success(response):
    doSuccess(response)
  case let .Failure(error):
    switch(error) {
    case .Timeout:
      // tip timeout
    case .Cancelled:
      // tip cancelled
    default:
      // tip unknown
    }
  }
}

does it more make sense?

[Jeehut]

@jasl Although I like the suggestion about improving the failure case with pre-defined cases, that still doesn't solve the issue that API errors (4xx/5xx) are still accessed via something simply called .Success. The confusion therefore wouldn't be reduced with Timeout and Cancelled failure cases imo.

[jasl]

@Dschee The definition of Result is Result<Response, Error>, means errors of API's shouldn't be handled, because you've got the response.

But your question is right, we need to handle API errors, for now Moya has provided some functions to help filter the status code, codes here, but those can't be used for pattern-matching. so we could do a small extension

public enum ResponseClass {
    case Informational
    case Success
    case Redirection
    case ClientError
    case ServerError
    case Undefined

    public init(statusCode: Int) {
        switch statusCode {
        case 100 ..< 200:
            self = .Informational
        case 200 ..< 300:
            self = .Success
        case 300 ..< 400:
            self = .Redirection
        case 400 ..< 500:
            self = .ClientError
        case 500 ..< 600:
            self = .ServerError
        default:
            self = .Undefined
        }
    }
}

public extension Response {
    public var responseClass: ResponseClass {
        return ResponseClass(statusCode: self.statusCode)
    }
}

then, we could

provider.request(.UserProfile) { result in
  switch result {
  case let .Success(response):
    switch response.responseClass {
    case .Success:
      doSuccess(response)
    case .ClientError:
      // tip user provided data is invalid
    default:
      // tip unexpected error
    }
  case let .Failure(error):
    switch(error) {
    case .Timeout:
      // tip timeout
    case .Cancelled:
      // tip cancelled
    default:
      // tip unknown
    }
  }
}

[Jeehut]

@jasl Thanks for adding cases for the success part as well. I continue to think those are great suggestions.

But again, that still doesn't solve the problem that the outer let .Success(response) case is named in a way people may be confused. To repeat the example with my colleague two days ago: I implemented a Moya provider class and he used it. But at the moment he wrote the first outer .Success and .Failure cases he already was convinced that he had handled API errors within the .Failure case and couldn't understand why the .Success case was called but didn't provide any data he was expecting from a successful API response.

To put it in code, here's what I would expect your code to look like:

provider.request(.UserProfile) { result in
  switch result {
  case let .Response(response):
    switch(response.responseClass) {
    case .Success:
      doSuccess(response)
    case .ClientError:
      // tip user provided data invalid
    default:
      // tip error
    }
  case let .Incomplete(error):
    switch(error) {
    case .Timeout:
      // tip timeout
    case .Cancelled:
      // tip cancelled
    default:
      // tip unknown
    }
  }
}

[colinta]

There might be a better name than "Success", but having it broken out based on the status code implies knowledge of the server. I've worked with an app (a terrible one), where all responses were "200", and other servers use 404 for empty "array resources".

But, instead of "Success", would "Response" be more appropriate?

On Mar 25, 2016, at 9:48 AM, Cihat Gündüz [email protected] wrote:

It just happened two days ago that one of my colleagues took over a project I implemented a new API class for using Moya and he was completely confused by the fact that an API error (regardless of 4xx or 5xx) was returned within the .Success case. Although I understand and like the fact that Moya only forwards network errors to the .Failure case what about renaming the generic .Success and .Failure cases to something more clear? For example simply adding Network as a prefix resulting to .NetworkSuccess and .NetworkFailure should already improve the clarity a lot. What do you think?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

[ashfurrow]

"Response" sounds appropriate. I don't want to impose semantics on top of Alamofire, so maybe have Response and Incomplete?

[jasl]

@Dschee your issue I also meet too, but I think there is two parts, one is the codes that I just provided is the right way (at least for me), the other part is this really make some people confusing.

In my opinion, there are too many kinds of errors could be occured, but we don't need to handle those one per one, let's regrouping them (remember, this depends on your business and your restful-api design)

enum APIResult {
    case Ok // the request with success but no content (e.g. status code 204)
    case Success(NSData) // success with content
    case Failure(APIError) // the request success but with 4xx, 5xx status
    case NetworkError(Moya.Error) // the request is failed
}

then I wrapped Moya's provider (just for demo)

func request(provider: Provider = Provider.defaultInstance(), completion: (result: APIResult) -> ()) -> Moya.Cancellable {
        return provider.request(self) { result in
            var apiResult: APIResult

            switch result {
            case let .Success(response):
                let json = response.mapSwiftyJSON()

                if json["ok"].int != nil {
                    apiResult = .Ok
                } else if let errorMessage = json["error"].string {
                    apiResult = .Failure(APIError(statusCode: response.statusCode, message: errorMessage))
                } else if let entity = parseResponse(json) {
                    apiResult = .Success(entity)
                } else {
                    apiResult = .Ok
                }
            case let .Failure(error):
                apiResult = .NetworkError(error)
            }

            completion(result: apiResult)
        }
    }

so my colleagues could use like

request(.UserProfile) { result in
  switch result {
  case .Ok:
  case let .Success(data):
  case let .Failure(error):
  case let .NetworkError(error):
  }
}

you could see my project for learning https://github.com/jasl/RubyChinaAPP/blob/master/RubyChinaApp/Controllers/Topics/TopicsViewController.swift#L127 (It's for Ruby Community of China, and the repo may not be passed to compiling because I using my forked Moya, but you could see how I wrapping Moya's Provider)

In my opinion, I don't like a library to do too much, if you think it's too complex to use, you can wrap it, that should be easy, but if the library to do too much, if there has any which not suit for your need, hacking it would be hard. I think Moya is a balance choice,

[scottrhoyt]

Since there hasn't been any activity on this thread for some time, I am going to go ahead and close it. Please re-open or create a new issue if you would like to discuss further. Thanks.