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

Propagate parameter encoding error for URLRequest mapping #1248

Merged
merged 10 commits into from Sep 26, 2017

Conversation

Projects
None yet
6 participants
@SD10
Member

SD10 commented Aug 31, 2017

Changes:

Adds:
MoyaError.parameterEncoding(Swift.Error) case

Refactors:
urlRequest -> urlRequest() (now throwing)

Summary of Pull Request:

As discussed in #1247 I saw that it would be really easy to pass on the errors thrown when attempting to encode URLRequest parameters.

All that had to be done is convert Endpoint's urlRequest property to a throwing function and pass the error to the .failure closure in MoyaProvider.defaultRequestMapping.

Personally, I'm not too fond of having to call try now when accessing an Endpoint's urlRequest. Therefore, I mostly consider this "research" unless any of you think there's value in it.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 31, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (10.0.0-dev@a684307). Click here to learn what that means.
The diff coverage is 78.72%.

Impacted file tree graph

@@              Coverage Diff              @@
##             10.0.0-dev    #1248   +/-   ##
=============================================
  Coverage              ?   83.98%           
=============================================
  Files                 ?       24           
  Lines                 ?      768           
  Branches              ?        0           
=============================================
  Hits                  ?      645           
  Misses                ?      123           
  Partials              ?        0
Impacted Files Coverage Δ
Sources/Moya/MoyaError.swift 28% <0%> (ø)
Sources/Moya/Plugins/NetworkActivityPlugin.swift 100% <100%> (ø)
Sources/Moya/MoyaProvider+Defaults.swift 76% <25%> (ø)
Sources/Moya/Endpoint.swift 93.22% <97.05%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a684307...42ecf86. Read the comment docs.

codecov-io commented Aug 31, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (10.0.0-dev@a684307). Click here to learn what that means.
The diff coverage is 78.72%.

Impacted file tree graph

@@              Coverage Diff              @@
##             10.0.0-dev    #1248   +/-   ##
=============================================
  Coverage              ?   83.98%           
=============================================
  Files                 ?       24           
  Lines                 ?      768           
  Branches              ?        0           
=============================================
  Hits                  ?      645           
  Misses                ?      123           
  Partials              ?        0
Impacted Files Coverage Δ
Sources/Moya/MoyaError.swift 28% <0%> (ø)
Sources/Moya/Plugins/NetworkActivityPlugin.swift 100% <100%> (ø)
Sources/Moya/MoyaProvider+Defaults.swift 76% <25%> (ø)
Sources/Moya/Endpoint.swift 93.22% <97.05%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a684307...42ecf86. Read the comment docs.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 1, 2017

Member

This is really cool, but I feel like this could be something we think about for version 10.0. We need to release 9.0 in a few. What do you think about it, @SD10?

Member

sunshinejr commented Sep 1, 2017

This is really cool, but I feel like this could be something we think about for version 10.0. We need to release 9.0 in a few. What do you think about it, @SD10?

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Sep 1, 2017

Member

@sunshinejr I have no problem delaying this until v10.0 or even not going with this solution at all

Member

SD10 commented Sep 1, 2017

@sunshinejr I have no problem delaying this until v10.0 or even not going with this solution at all

@sunshinejr sunshinejr referenced this pull request Sep 2, 2017

Closed

Release 10.0 #1253

9 of 11 tasks complete

@sunshinejr sunshinejr closed this Sep 4, 2017

@sunshinejr sunshinejr changed the base branch from 9.0.0-dev to master Sep 4, 2017

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 4, 2017

Member

Damn, GitHub did let me remove the 9.0.0-dev branch while PRs were targeted to it still. Reopening!

Member

sunshinejr commented Sep 4, 2017

Damn, GitHub did let me remove the 9.0.0-dev branch while PRs were targeted to it still. Reopening!

@sunshinejr sunshinejr reopened this Sep 4, 2017

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 16, 2017

Member

Hey @SD10, would you be up for finishing that one? Conflicts + Docs + Changelog is there to finish I believe? Other than that I think this looks good!

Member

sunshinejr commented Sep 16, 2017

Hey @SD10, would you be up for finishing that one? Conflicts + Docs + Changelog is there to finish I believe? Other than that I think this looks good!

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Sep 17, 2017

Member

@sunshinejr Yes, I will finish this up. I need about 2 days though

Member

SD10 commented Sep 17, 2017

@sunshinejr Yes, I will finish this up. I need about 2 days though

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 18, 2017

Member

Great! Take your time 👍

Member

sunshinejr commented Sep 18, 2017

Great! Take your time 👍

SD10 added a commit that referenced this pull request Sep 22, 2017

@SD10 SD10 changed the base branch from master to 10.0.0-dev Sep 22, 2017

@MoyaBot

This comment has been minimized.

Show comment
Hide comment
@MoyaBot

MoyaBot Sep 22, 2017

SwiftLint found issues

Warnings

File Line Reason
EndpointSpec.swift 245 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
EndpointSpec.swift 246 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
MoyaProviderSpec.swift 975 File should contain 400 lines or less: currently contains 975

Errors

File Line Reason
MoyaProviderSpec.swift 7 Type body should span 350 lines or less excluding comments and whitespace: currently spans 594 lines

Generated by 🚫 Danger

MoyaBot commented Sep 22, 2017

SwiftLint found issues

Warnings

File Line Reason
EndpointSpec.swift 245 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
EndpointSpec.swift 246 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
MoyaProviderSpec.swift 975 File should contain 400 lines or less: currently contains 975

Errors

File Line Reason
MoyaProviderSpec.swift 7 Type body should span 350 lines or less excluding comments and whitespace: currently spans 594 lines

Generated by 🚫 Danger

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Sep 22, 2017

Member

That's what I get for trying to resolve a merge conflict in nano 😃

Member

SD10 commented Sep 22, 2017

That's what I get for trying to resolve a merge conflict in nano 😃

@SD10 SD10 changed the title from [WIP] Propagate parameter encoding error for URLRequest mapping to Propagate parameter encoding error for URLRequest mapping Sep 22, 2017

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 23, 2017

Member

@SD10 would you please rebase & fix the conflicts? I've merged few other PRs in the meantime.

Member

sunshinejr commented Sep 23, 2017

@SD10 would you please rebase & fix the conflicts? I've merged few other PRs in the meantime.

SD10 added a commit that referenced this pull request Sep 23, 2017

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Sep 23, 2017

Member

@sunshinejr Sure thing. Should be good to go if resolving the conflicts went well and CI is all green. Unless anyone else has any input/review. Giving it another look over myself now

Member

SD10 commented Sep 23, 2017

@sunshinejr Sure thing. Should be good to go if resolving the conflicts went well and CI is all green. Unless anyone else has any input/review. Giving it another look over myself now

@sunshinejr

Thanks, @SD10! Seems good to me, but I think it would be awesome to get another pair of eyes on that one.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Sep 23, 2017

Member

Yes, I'd really appreciate it if another @Moya/contributors gave their input. The goal of this PR is to expose more information about the failure of parameter encoding for a request.

I don't want to hear a ton of complaints about having to use try syntax when accessing the urlRequest for an Endpoint if this were to be merged.

Member

SD10 commented Sep 23, 2017

Yes, I'd really appreciate it if another @Moya/contributors gave their input. The goal of this PR is to expose more information about the failure of parameter encoding for a request.

I don't want to hear a ton of complaints about having to use try syntax when accessing the urlRequest for an Endpoint if this were to be merged.

@ashfurrow

Looks good to me 👍 From an API perspective I think this makes a lot more sense, and migration is pretty easy. Good job!

let rhsRequest = try? rhs.urlRequest()
if lhsRequest != nil, rhsRequest == nil { return false }
if lhsRequest == nil, rhsRequest != nil { return false }
if lhsRequest == nil, rhsRequest == nil { return lhs.hashValue == rhs.hashValue }

This comment has been minimized.

@ashfurrow

ashfurrow Sep 23, 2017

Member

This is totally fine, though I will point out that if two different Endpoints both fail to produce a urlRequest, then they would be "equal". I'm sure that's fine, especially since it's consistent with hashValue. The only consequences would be using an Endpoint as key in a dictionary, or in a Set. I think it's worth a comment, but otherwise this is totally 👍

@ashfurrow

ashfurrow Sep 23, 2017

Member

This is totally fine, though I will point out that if two different Endpoints both fail to produce a urlRequest, then they would be "equal". I'm sure that's fine, especially since it's consistent with hashValue. The only consequences would be using an Endpoint as key in a dictionary, or in a Set. I think it's worth a comment, but otherwise this is totally 👍

This comment has been minimized.

@SD10

SD10 Sep 24, 2017

Member

If two different Endpoints both fail to produce a urlRequest it will compare both endpoints using the hashValue of the url: String property.

So if two different Endpoints both fail to produce a urlRequest and they have the same String for the url -- they will be considered equal. I'll add a comment noting this behavior.

@SD10

SD10 Sep 24, 2017

Member

If two different Endpoints both fail to produce a urlRequest it will compare both endpoints using the hashValue of the url: String property.

So if two different Endpoints both fail to produce a urlRequest and they have the same String for the url -- they will be considered equal. I'll add a comment noting this behavior.

@@ -72,8 +72,8 @@ final class EndpointSpec: QuickSpec {
it("returns a nil urlRequest for an invalid URL") {
let badEndpoint = Endpoint<Empty>(url: "some invalid URL", sampleResponseClosure: { .networkResponse(200, Data()) }, method: .get, task: .requestPlain, httpHeaderFields: nil)
expect(badEndpoint.urlRequest).to(beNil())
let urlRequest = try? badEndpoint.urlRequest()

This comment has been minimized.

@ashfurrow

ashfurrow Sep 23, 2017

Member

This is good – can we also add a test that makes sure the error thrown is correct?

@ashfurrow

ashfurrow Sep 23, 2017

Member

This is good – can we also add a test that makes sure the error thrown is correct?

This comment has been minimized.

@SD10

SD10 Sep 24, 2017

Member

Great idea 👍 Thanks for the input

@SD10

SD10 Sep 24, 2017

Member

Great idea 👍 Thanks for the input

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Sep 24, 2017

Member

As per @ashfurrow 's feedback I added tests to make sure urlRequest is throwing the correct error.

I changed the implementation to throw the .parameterEncoding error directly from urlRequest() instead of wrapping it up in defaultEndpointMapping. This is because I was receiving an Alamofire error instead of a MoyaError through urlRequest. Nice how writing a test caught this 😉

Truthfully, it was extremely hard to write a test that caught an error -- it even made me question the value of this change.

This error will only ever be triggered when JSONSerialization fails to serialize the parameters. I went hunting in core-libs-foundation to see what errors it throws -- but the majority of them aren't caught gracefully and come back as an NSInvalidArgumentException.

Member

SD10 commented Sep 24, 2017

As per @ashfurrow 's feedback I added tests to make sure urlRequest is throwing the correct error.

I changed the implementation to throw the .parameterEncoding error directly from urlRequest() instead of wrapping it up in defaultEndpointMapping. This is because I was receiving an Alamofire error instead of a MoyaError through urlRequest. Nice how writing a test caught this 😉

Truthfully, it was extremely hard to write a test that caught an error -- it even made me question the value of this change.

This error will only ever be triggered when JSONSerialization fails to serialize the parameters. I went hunting in core-libs-foundation to see what errors it throws -- but the majority of them aren't caught gracefully and come back as an NSInvalidArgumentException.

it("throws a .parameterEncoding error") {
// Non-serializable type to cause serialization error
class InvalidParameter {}

This comment has been minimized.

@ashfurrow

ashfurrow Sep 24, 2017

Member

Nice.

@ashfurrow

ashfurrow Sep 24, 2017

Member

Nice.

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Sep 24, 2017

Member

Awesome, great work! Let's leave it a few days in case anyone else has input.

@Moya/core-team I wonder if we should target a new moya-10 branch with the PR, to keep master for any non-breaking fixes we can release as 9.0.x patches. Anyone have thoughts on a branching strategy?

Member

ashfurrow commented Sep 24, 2017

Awesome, great work! Let's leave it a few days in case anyone else has input.

@Moya/core-team I wonder if we should target a new moya-10 branch with the PR, to keep master for any non-breaking fixes we can release as 9.0.x patches. Anyone have thoughts on a branching strategy?

@pedrovereza

This comment has been minimized.

Show comment
Hide comment
@pedrovereza

pedrovereza Sep 24, 2017

Member

@ashfurrow This PR is already targeting branch 10.0.0-dev 😉

Member

pedrovereza commented Sep 24, 2017

@ashfurrow This PR is already targeting branch 10.0.0-dev 😉

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Sep 24, 2017

Member

Oops 🙈

Member

ashfurrow commented Sep 24, 2017

Oops 🙈

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 26, 2017

Member

I was rebasing 10.0.0-dev again as we had documentation changes, so I rebased this one as well. Sorry for trouble.

Member

sunshinejr commented Sep 26, 2017

I was rebasing 10.0.0-dev again as we had documentation changes, so I rebased this one as well. Sorry for trouble.

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Sep 26, 2017

Member

Cool, looks good to merge on 🍏

Member

ashfurrow commented Sep 26, 2017

Cool, looks good to merge on 🍏

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 26, 2017

Member

Great! Thanks for all your hard work on that one, @SD10 😉

Member

sunshinejr commented Sep 26, 2017

Great! Thanks for all your hard work on that one, @SD10 😉

@sunshinejr sunshinejr merged commit 7c74420 into 10.0.0-dev Sep 26, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details

@sunshinejr sunshinejr deleted the feature/parameterEncoding-error branch Sep 26, 2017

sunshinejr added a commit that referenced this pull request Oct 2, 2017

AndrewSB added a commit that referenced this pull request Oct 3, 2017

sunshinejr added a commit that referenced this pull request Oct 8, 2017

sunshinejr added a commit that referenced this pull request Oct 8, 2017

sunshinejr added a commit that referenced this pull request Oct 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment