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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DataPreprocessor API #2903

Merged
merged 6 commits into from Aug 14, 2019

Conversation

@jshier
Copy link
Contributor

commented Aug 12, 2019

Issue Link 馃敆

This is a generalized solution to #2683.

Goals 鈿斤笍

This PR adds the DataPreprocessor API to all response serializers, to allow easy access to raw Data before serialization, to do things like stripping XSSI strings.

Implementation Details 馃毀

DataPreprocessor is just a closure that each serializer can have set. By default it just passes Data through, so it should have minimal impact for anyone not using the API. It also supports throwing, so errors can be returned as well.

Testing Details 馃攳

Additional tests were added to the various DataResponseSerializer cases, as all serialization flows through those methods.

@jshier

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Design question: is the closure good enough, or should this be full abstracted into a protocol?

@jshier jshier requested a review from cnoon Aug 12, 2019

@jshier jshier added this to the 5.0.0.rc.1 milestone Aug 12, 2019

@cnoon
cnoon approved these changes Aug 13, 2019
Copy link
Member

left a comment

I think this is a really good addition @jshier. I had some comments about possibly switching to a protocol, but the change itself looks great.

@@ -64,18 +64,25 @@ public protocol DownloadResponseSerializerProtocol {

/// A serializer that can handle both data and download responses.
public protocol ResponseSerializer: DataResponseSerializerProtocol & DownloadResponseSerializerProtocol {
/// Closure type used to prepare `Data` for serialization.
typealias DataPreprocessor = (Data) throws -> Data

This comment has been minimized.

Copy link
@cnoon

cnoon Aug 13, 2019

Member

I really like this change overall. It really gives you a lot of flexibility if you need it. I think for users that actually need this functionality though might be better served by this being a protocol rather than a closure. I think most people would want to define an explicit type to very easily test rather than having to implement it as a closure. I just think it could be some fairly complex logic that people may want to reuse more easily. They could pass functions off as the closure, but that doesn't seem ideal.

In the XSSI issue, it seems like it would be nice to be able to create an XSSIDataPreprocessor: DataPreprocessor that you could extensively unit test and leverage throughout your code. So I think I lean towards having DataPreprocessor be a protocol rather than a typealias'd closure. Either way, both solutions are solid.

This comment has been minimized.

Copy link
@jshier

jshier Aug 13, 2019

Author Contributor

I updated to a protocol in 6d1a440.

@@ -398,33 +405,37 @@ extension DataRequest {
/// request returning `nil` or no data is considered an error. However, if the response is has a status code valid for
/// empty responses (`204`, `205`), then an empty `Data` value is returned.
public final class DataResponseSerializer: ResponseSerializer {
/// HTTP response codes for which empty responses are allowed.
public let dataPreprocessor: DataPreprocessor

This comment has been minimized.

Copy link
@cnoon

cnoon Aug 13, 2019

Member

Does dataPreprocessor have a place in the top-level response chainable APIs, or is it such an advanced feature that it only warrants customization at the response serializer initializers?

Also, would you expect people to create their own chainable extensions when using these?

Not that this matters so much for this PR, but just curious in general.

This comment has been minimized.

Copy link
@jshier

jshier Aug 13, 2019

Author Contributor

That could be the next step for this API, but I don't think it will be used much, so I don't think it's valuable as a chainable API. Customizing the existing serializers should be enough for what users need here.

jshier added 2 commits Aug 13, 2019
@jshier

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@cnoon I've refactored DataPreprocessor into a protocol, take a look! I think it's okay, but would be far better but for a Swift limitation around being able to use static members of protocols.

@cnoon
cnoon approved these changes Aug 13, 2019
Copy link
Member

left a comment

Looks good @jshier, just a couple questions back to you that you can take or leave.

func preprocess(_ data: Data) throws -> Data {
return (data.prefix(6) == Data(")]}',\n".utf8)) ? data.dropFirst(6) : data
}
}

This comment has been minimized.

Copy link
@cnoon

cnoon Aug 13, 2019

Member

I'm not sure this XSSI type is actually being used at all right now. Also, would it make sense to expose Passthrough as a public type? Would it be convenient for clients, or would you just expect those building custom response serializers to use ResponseSerializer.defaultDataPreprocessor?

This comment has been minimized.

Copy link
@cnoon

cnoon Aug 13, 2019

Member

If either of these do get made public, I think I'd recommend tacking on a suffix like Preprocessor or DataPreprocessor so it's a bit more clear what they are.

This comment has been minimized.

Copy link
@jshier

jshier Aug 13, 2019

Author Contributor

I've made them both public, updated the naming, and added some tests.

jshier added 3 commits Aug 13, 2019

@jshier jshier merged commit 01c4092 into master Aug 14, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jshier jshier deleted the feature/data-preprocessing-api branch Aug 14, 2019

toomasr added a commit to toomasr/Alamofire that referenced this pull request Aug 19, 2019
Add DataPreprocessor API (Alamofire#2903)
* Add DataPreprocessor API.

* Make DataPreprocessor a protocol.

* Cleanup whitespace.

* Rename included preprocessors, make public, add tests.

* Add test.

* Update documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.