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

Fix base value types(String, Int, Bool) mapping cause crash. #1405

Merged
merged 10 commits into from Nov 13, 2017

Conversation

Projects
None yet
9 participants
@ufosky
Contributor

ufosky commented Oct 25, 2017

Currently on the release version 10.0.0, The response data mapping to base value types(String, Int, Bool) at given key path will cause crash. This PR fix these cases, but cannot mapping to other types like URL, Date etc.

@MoyaBot

This comment has been minimized.

Show comment
Hide comment
@MoyaBot

MoyaBot Oct 25, 2017

2 Warnings
⚠️ Big PR, try to keep changes smaller if you can
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

SwiftLint found issues

Warnings

File Line Reason
Observable+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 318 lines
Observable+MoyaSpec.swift 492 File should contain 400 lines or less: currently contains 492
SignalProducer+MoyaSpec.swift 10 Type body should span 200 lines or less excluding comments and whitespace: currently spans 302 lines
SignalProducer+MoyaSpec.swift 474 File should contain 400 lines or less: currently contains 474
Single+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 306 lines
Single+MoyaSpec.swift 484 File should contain 400 lines or less: currently contains 484

Generated by 🚫 Danger

MoyaBot commented Oct 25, 2017

2 Warnings
⚠️ Big PR, try to keep changes smaller if you can
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

SwiftLint found issues

Warnings

File Line Reason
Observable+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 318 lines
Observable+MoyaSpec.swift 492 File should contain 400 lines or less: currently contains 492
SignalProducer+MoyaSpec.swift 10 Type body should span 200 lines or less excluding comments and whitespace: currently spans 302 lines
SignalProducer+MoyaSpec.swift 474 File should contain 400 lines or less: currently contains 474
Single+MoyaSpec.swift 6 Type body should span 200 lines or less excluding comments and whitespace: currently spans 306 lines
Single+MoyaSpec.swift 484 File should contain 400 lines or less: currently contains 484

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 25, 2017

Codecov Report

Merging #1405 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1405   +/-   ##
=======================================
  Coverage   88.41%   88.41%           
=======================================
  Files           5        5           
  Lines         164      164           
=======================================
  Hits          145      145           
  Misses         19       19

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 8d2a9dd...3b4cf9f. Read the comment docs.

codecov-io commented Oct 25, 2017

Codecov Report

Merging #1405 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1405   +/-   ##
=======================================
  Coverage   88.41%   88.41%           
=======================================
  Files           5        5           
  Lines         164      164           
=======================================
  Hits          145      145           
  Misses         19       19

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 8d2a9dd...3b4cf9f. Read the comment docs.

@BasThomas BasThomas requested a review from sunshinejr Oct 25, 2017

@BasThomas

This comment has been minimized.

Show comment
Hide comment
@BasThomas

BasThomas Oct 25, 2017

Contributor

Thanks for looking into this @ufosky! It seems like this would a breaking change, right?

Contributor

BasThomas commented Oct 25, 2017

Thanks for looking into this @ufosky! It seems like this would a breaking change, right?

@BasThomas

This comment has been minimized.

Show comment
Hide comment
@BasThomas

BasThomas Oct 25, 2017

Contributor

Also I think this should go into master, right @sunshinejr? When / where do we use the development branch even? I was surprised to see it was even with master. 🤔

Contributor

BasThomas commented Oct 25, 2017

Also I think this should go into master, right @sunshinejr? When / where do we use the development branch even? I was surprised to see it was even with master. 🤔

@ufosky

This comment has been minimized.

Show comment
Hide comment
@ufosky

ufosky Oct 25, 2017

Contributor

@BasThomas It isn't a breaking change, since in this code if jsonObject is not an array or dictionary, it causes a crash, so it's a bug.

jsonData = try JSONSerialization.data(withJSONObject: jsonObject)
Contributor

ufosky commented Oct 25, 2017

@BasThomas It isn't a breaking change, since in this code if jsonObject is not an array or dictionary, it causes a crash, so it's a bug.

jsonData = try JSONSerialization.data(withJSONObject: jsonObject)
@sunshinejr

Huh, this is really a great finding, @ufosky, thanks! I'm surprised that it doesn't throw an exception but just crashes. We should file a radar if it wasn't already, that's for sure, because it actually handles exceptions already.
I made a few comments, let me know if these make sense to you!

@BasThomas I will create an issue for our git "flow" in a few, seems like it was my mistake to not discuss it before, sorry for that.

Show outdated Hide outdated Sources/Moya/Response.swift Outdated
Show outdated Hide outdated Tests/Observable+MoyaSpec.swift Outdated
Show outdated Hide outdated Tests/Observable+MoyaSpec.swift Outdated
Show outdated Hide outdated Tests/Observable+MoyaSpec.swift Outdated
@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 25, 2017

Member

I also assigned you @devxoul, would love to have your input in it, but no pressure of course :)

Member

sunshinejr commented Oct 25, 2017

I also assigned you @devxoul, would love to have your input in it, but no pressure of course :)

@ufosky

This comment has been minimized.

Show comment
Hide comment
@ufosky

ufosky Oct 26, 2017

Contributor

Maybe we can check the value at a key path is string ,int,bool or null,then convert it to data,this way may support URL ,Date or other Decodable types.

Contributor

ufosky commented Oct 26, 2017

Maybe we can check the value at a key path is string ,int,bool or null,then convert it to data,this way may support URL ,Date or other Decodable types.

@ufosky

This comment has been minimized.

Show comment
Hide comment
@ufosky

ufosky Oct 26, 2017

Contributor

I update the code to support all Decodable types, and modify the test cases follow your advices.

Contributor

ufosky commented Oct 26, 2017

I update the code to support all Decodable types, and modify the test cases follow your advices.

Show outdated Hide outdated Sources/Moya/Response.swift Outdated
@@ -142,3 +158,7 @@ public extension Response {
}
}
}
private struct DecodableWrapper<T: Decodable>: Decodable {

This comment has been minimized.

@SD10

SD10 Oct 26, 2017

Member

Can you explain why we had to add this wrapper?

@SD10

SD10 Oct 26, 2017

Member

Can you explain why we had to add this wrapper?

This comment has been minimized.

@ufosky

ufosky Oct 26, 2017

Contributor

Because the base value types cannot serialize to json data, so I wrap the value to a Dictionary, then serialize the Dictionary to Data. So when decode the data, the result should be unwrapped.

@ufosky

ufosky Oct 26, 2017

Contributor

Because the base value types cannot serialize to json data, so I wrap the value to a Dictionary, then serialize the Dictionary to Data. So when decode the data, the result should be unwrapped.

@devxoul

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Oct 26, 2017

Member

We have both RxSwift and ReactiveSwift extensions:

  • Observable
  • Single
  • SignalProducer

Could you please write a test for SignalProducer?

Member

devxoul commented Oct 26, 2017

We have both RxSwift and ReactiveSwift extensions:

  • Observable
  • Single
  • SignalProducer

Could you please write a test for SignalProducer?

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 26, 2017

Member

We need to add a CHANGELOG entry for this under Fixed.

For anyone who is interested this is the intended behavior so crashing instead of throwing is not a bug.

Using isValidJSONObject will protect us from this 😃

Member

SD10 commented Oct 26, 2017

We need to add a CHANGELOG entry for this under Fixed.

For anyone who is interested this is the intended behavior so crashing instead of throwing is not a bug.

Using isValidJSONObject will protect us from this 😃

@ufosky

This comment has been minimized.

Show comment
Hide comment
@ufosky

ufosky Oct 26, 2017

Contributor

@SD10 Should bump the next release version before add change log entries.

Contributor

ufosky commented Oct 26, 2017

@SD10 Should bump the next release version before add change log entries.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 27, 2017

Member

@ufosky We add new changelog entries under the Next Release section without a specified version.
You can refer to our changelog guidelines if this helps 👍

Member

SD10 commented Oct 27, 2017

@ufosky We add new changelog entries under the Next Release section without a specified version.
You can refer to our changelog guidelines if this helps 👍

@ufosky

This comment has been minimized.

Show comment
Hide comment
@ufosky

ufosky Oct 27, 2017

Contributor

Do we need a method likes below, it returns nil if the keyPath is not present in the response data, just like the method decodeIfPresent(_:forKey:) in protocol KeyedDecodingContainerProtocol:

func mapIfPresent<D: Decodable>(_ type: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder = JSONDecoder()) throws -> D?
Contributor

ufosky commented Oct 27, 2017

Do we need a method likes below, it returns nil if the keyPath is not present in the response data, just like the method decodeIfPresent(_:forKey:) in protocol KeyedDecodingContainerProtocol:

func mapIfPresent<D: Decodable>(_ type: D.Type, atKeyPath keyPath: String? = nil, using decoder: JSONDecoder = JSONDecoder()) throws -> D?
@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 27, 2017

Member

@ufosky The current decoding method should handle a case where the key is invalid, correct? We could consider propagating a more descriptive error regarding which point in the process the decoding failed, but I don't think we need the overhead of an additional method.

Member

SD10 commented Oct 27, 2017

@ufosky The current decoding method should handle a case where the key is invalid, correct? We could consider propagating a more descriptive error regarding which point in the process the decoding failed, but I don't think we need the overhead of an additional method.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 28, 2017

Member

@ufosky can you also rebase and change this PR's target into master, please? This is a fix we would like to include in the nearest patch release.

Member

sunshinejr commented Oct 28, 2017

@ufosky can you also rebase and change this PR's target into master, please? This is a fix we would like to include in the nearest patch release.

@ufosky

This comment has been minimized.

Show comment
Hide comment
@ufosky

ufosky Oct 30, 2017

Contributor

@sunshinejr I don't find the correct way to use rebasing. Can I cherry pick the commits into master?

Contributor

ufosky commented Oct 30, 2017

@sunshinejr I don't find the correct way to use rebasing. Can I cherry pick the commits into master?

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Oct 30, 2017

Member

@ufosky You can always just resolve this conflict on GitHub. It's just a CHANGELOG conflict. We need to keep the both of them. It will be a little easier to rebase once you don't have to work from a fork 😅

Member

SD10 commented Oct 30, 2017

@ufosky You can always just resolve this conflict on GitHub. It's just a CHANGELOG conflict. We need to keep the both of them. It will be a little easier to rebase once you don't have to work from a fork 😅

@SD10 SD10 changed the base branch from development to master Oct 30, 2017

ufosky added some commits Oct 30, 2017

@SD10

SD10 approved these changes Nov 13, 2017

I just gave this another look over. The body of the decodable method has become quite crazy, but I don't think it's possible to simplify it further.

I think we should try and get this bugfix merged before it becomes stale. Can anyone else sign off on it @Moya/contributors?

@freak4pc

Minor styling nit but looks good to me bedsides

LGTM

Show outdated Hide outdated Sources/Moya/Response.swift Outdated
@freak4pc

This comment has been minimized.

Show comment
Hide comment
@freak4pc

freak4pc Nov 13, 2017

Member

Regarding rebasing - since this is just CHANGELOG you can just copy it from master and apply your changes over it.

Member

freak4pc commented Nov 13, 2017

Regarding rebasing - since this is just CHANGELOG you can just copy it from master and apply your changes over it.

@sunshinejr

Looks good to me, the only change I'd request would be the one from @freak4pc about guard, but after that it seems like it's ready to merge! 🎉

@freak4pc

This comment has been minimized.

Show comment
Hide comment
@freak4pc

freak4pc Nov 13, 2017

Member

@sunshinejr I would strongly suggest moving all the let thing = try something(x) { } else { throwError } to a separate tryOrThrow method similar to RxSwift, if you know that one. I think it would really clean up that code. As @SD10 suggested it has became a bit monstrously sized :-P Don't mind making the PR if you'd be interested.

Member

freak4pc commented Nov 13, 2017

@sunshinejr I would strongly suggest moving all the let thing = try something(x) { } else { throwError } to a separate tryOrThrow method similar to RxSwift, if you know that one. I think it would really clean up that code. As @SD10 suggested it has became a bit monstrously sized :-P Don't mind making the PR if you'd be interested.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 13, 2017

Member

@freak4pc yeah, if you have some spare time it would be awesome if you could do a PR. I was talking about the guard in this PR only 😄

Member

sunshinejr commented Nov 13, 2017

@freak4pc yeah, if you have some spare time it would be awesome if you could do a PR. I was talking about the guard in this PR only 😄

@ufosky

This comment has been minimized.

Show comment
Hide comment
@ufosky

ufosky Nov 13, 2017

Contributor

Is there any tryOrThrow implementation in Moya?

Contributor

ufosky commented Nov 13, 2017

Is there any tryOrThrow implementation in Moya?

@freak4pc

This comment has been minimized.

Show comment
Hide comment
@freak4pc

freak4pc Nov 13, 2017

Member

@ufosky sorry I kinda got confused with the specific castOrThrow etc. This will need a bit more though. Anyways, this PR "as it is" should be good to Go IMO. @sunshinejr care to merge?

Member

freak4pc commented Nov 13, 2017

@ufosky sorry I kinda got confused with the specific castOrThrow etc. This will need a bit more though. Anyways, this PR "as it is" should be good to Go IMO. @sunshinejr care to merge?

@sunshinejr sunshinejr merged commit ea236dc into Moya:master Nov 13, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Nov 13, 2017

Member

Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions.

Member

ashfurrow commented Nov 13, 2017

Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 13, 2017

Member

Thank you very much for your hard work, @ufosky! 👍

Member

sunshinejr commented Nov 13, 2017

Thank you very much for your hard work, @ufosky! 👍

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 13, 2017

Member

Thanks for jumping in so quickly @freak4pc @sunshinejr and of course @ufosky for the great work 👍

Member

SD10 commented Nov 13, 2017

Thanks for jumping in so quickly @freak4pc @sunshinejr and of course @ufosky for the great work 👍

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