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

[REQ] [Swift] Abstract away URLSession #11651

Closed
Czajnikowski opened this issue Feb 18, 2022 · 9 comments
Closed

[REQ] [Swift] Abstract away URLSession #11651

Czajnikowski opened this issue Feb 18, 2022 · 9 comments

Comments

@Czajnikowski
Copy link

Czajnikowski commented Feb 18, 2022

Is your feature request related to a problem? Please describe.

Yes. I'm trying to implement a caching mechanism that basically overrides the URLSession.dataTask and under the hood it takes the response from the cache (if available) and at the same time loads using the URLSession itself. Currently, the only way of changing the URLSession is via override of URLSessionRequestBuilder.createURLSession (well, I can always write my own RequestBuilder, but that'd be a lot of duplicated code). So to be able to implement my approach I'd have to subclass URLSession and override the dataTask.

The implementation would look like this:

class CacheFirstURLSession: URLSession {
    override func dataTask(
        with request: URLRequest,
        completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void
    ) -> URLSessionDataTask {
        if let cachedResponse = configuration.urlCache?.cachedResponse(for: request) {
            completionHandler(cachedResponse.data, cachedResponse.response, nil)
        }

        return super.dataTask(with: request, completionHandler: completionHandler)
    }
}

This is not currently doable (as far as I know, URLSessions are not subclassable). I tried a couple of times to make a subclass, but it does not work - calling dataTask crashes the app. Probably because there's no way to inject configuration into a subclass.

Describe the solution you'd like

So I don't necessarily need a subclass of the URLSession, I can go with a wrapper like :

class URLSessionWrapper {
    private let wrapped: URLSession
    
    init(with wrapped: URLSession) {
        self.wrapped = wrapped
    }
}

extension URLSessionWrapper: DataTaskMaking {
    func dataTask(
        with request: URLRequest,
        completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void
    ) -> URLSessionDataTask {
        if let cachedResponse = wrapped.configuration.urlCache?.cachedResponse(for: request) {
            completionHandler(cachedResponse.data, cachedResponse.response, nil)
        }

        return wrapped.dataTask(with: request, completionHandler: completionHandler)
    }
}

The only problem that I have now is that the URLSessionRequestBuilder.createURLSession returns the exact URLSession. If we'd be able to exchange it into a protocol then my problem is solved.

Here's an initial idea of what we'd have to do in the URLSessionImplementation.mustache:

Somewhere at the top (or bottom):

public protocol DataTaskMaking {
    func dataTask(
        with request: URLRequest,
        completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void
    ) -> URLSessionDataTask
}

extension URLSession: DataTaskMaking {}

And then change the override:

{...} func createURLSession() -> DataTaskMaking {
    return defaultURLSession
}

Describe alternatives you've considered

So I tried solving the problem with subclassing and extensions which both don't seem to be feasible in our current architecture. Another possible option is to create my own own RequestBuilder, but that'd be a lot of duplicated code.

Additional context

Nope

@leszek-s
Copy link
Contributor

leszek-s commented Apr 8, 2022

Yes, abstracting URLSession with a protocol as proposed by @Czajnikowski would be super useful not only for implementing caching as proposed here but also for other stuff such as mocking for unit tests, adding semaphores or some kind of other mechanism to limit the number of request executed at the same time, adding logs, adding some additional headers only to specific requests and generally that would give much more flexibility to existing templates for people who need that flexibility. I'm experimenting with openapi generator since few days because I plan to use it in next project and that URLSession abstraction would greatly simplify a lot of stuff for me. Without this abstraction currently lots of stuff seems to be very inconvenient to implement and would require duplicating and overriding a lot code from current template or using custom modified template. This URLSession abstraction would be a massive improvement and it seems that it would not require many changes in the code. @4brunu is there any chance that this can be added to swift5 template?

@4brunu
Copy link
Contributor

4brunu commented Apr 8, 2022

Hey,
This is already possible today.
You need to subclass URLSessionRequestBuilder and URLSessionDecodableRequestBuilder and you override the method createURLSession to return your desire subclass of URLSession.

Here is an example of subclassing URLSessionRequestBuilder and URLSessionDecodableRequestBuilder.

https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/swift5/urlsessionLibrary/SwaggerClientTests/SwaggerClient/BearerDecodableRequestBuilder.swift

If you do this you have a lot of customisation available, but it's not well documented, I learned it by exploring the code and implement some things like you are requesting.

By overriding the method createURLSession you can use your own subclass of URLSession.

By overriding the method execute you can control what happens before each request is executed, avoid that it executes, return a cached response, etc.

Hope that it helps.

@leszek-s
Copy link
Contributor

leszek-s commented Apr 8, 2022

Hello @4brunu, I saw that example in BearerDecodableRequestBuilder.swift and yes that works for implementing token refreshing but still subclassing URLSession would be better but unfortunately it is not possible. As @Czajnikowski wrote subclassing func dataTask(with request: URLRequest, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask crashes the app with SIGABORT error. I think it was possible to subclass URLSession some time ago but currently it is not working I just checked with this code which always crashes on call to super.dataTask:

class MyURLSession: URLSession {
    static let defaultUrlSession = MyURLSession()
    override func dataTask(with request: URLRequest, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {
        return super.dataTask(with: request, completionHandler: completionHandler)
    }
}

and

    override func createURLSession() -> URLSession {
        return MyURLSession.defaultUrlSession
    }

It also shows a warning that 'init()' was deprecated in iOS 13.0 next to MyURLSession() and if you open URLSession definition you can also see this in Apple's code:

    public /*not inherited*/ init(configuration: URLSessionConfiguration)

    public /*not inherited*/ init(configuration: URLSessionConfiguration, delegate: URLSessionDelegate?, delegateQueue queue: OperationQueue?)

So it is currently not possible to properly initialize subclassed URLSession and that explains why it crashes if you try to do that. Therefore without abstracting URLSession replacing it by overriding createURLSession is currently not very useful. Also this stack overflow topic confirms problems with subclassing URLSession: https://stackoverflow.com/questions/48158484/subclassing-factory-methods-of-urlsession-in-swift
If instead of URLSession we could provide anything that confirms to a protocol as proposed by @Czajnikowski then that would solve this problem.

@4brunu
Copy link
Contributor

4brunu commented Apr 11, 2022

I see the issue.
I'm open to this solution, and will help with the review process, if someone want's to open a PR.

@leszek-s
Copy link
Contributor

@4brunu I pushed changes that work for me.

@4brunu
Copy link
Contributor

4brunu commented Apr 11, 2022

Could you please open a PR?

@Czajnikowski
Copy link
Author

Thanks for reviving that thread and for the implementation @leszek-s 🙌

@leszek-s
Copy link
Contributor

Could you please open a PR?

Here is PR: #12110

leszek-s added a commit to leszek-s/openapi-generator that referenced this issue Apr 19, 2022
@4brunu
Copy link
Contributor

4brunu commented Apr 19, 2022

fixed by #12110

@4brunu 4brunu closed this as completed Apr 19, 2022
rk0n pushed a commit to rk0n/openapi-generator that referenced this issue Apr 24, 2022
spacether added a commit that referenced this issue Apr 26, 2022
* Add source folder variable to fastapi fix 12118

* Add generated sample files for python-fastapi fix 12118

* [python-experimental] fixes json + charset use case (#12114)

* Adds code to detect json content type when charset is also set

* Updates template to properly render content type, regenerates samples

* Adds test_json_with_charset

* Reverts version file

* Fixes typo

* Add example allOf with single ref (#10948)

* Add example allOf with single ref

* fix dart-dio-next handling of that case

* Refactor without vendor extension

* Regenerate newer samples

* Add a sample of an enum model array in query params (#12107)

* [typescript-fetch] drop support typescript under v4.0 (#12102)

* [typescript-fetch] drop support typescript under v4.0

* [typescript-fetch] update docs

* [typescript-fetch] update package-lock.json manually & fix test

* [typescript-fetch] fix test

* update samples

* [dart] Remove old dio generator (to be replaced with dart-dio-next) (#12109)

* remove old `dart-dio` generator which will be replaced by `dart-dio-next` in a seperate PR
* remove left-over `DartJaguarClinetCodegen` class which was sunset a while ago and is unused

* fix: respect configured generator URL in swagger config (#12064)

* fix: respect configured generator URL in swagger config

The generated OpenAPI spec does not reflect the GENERATOR_HOST which causes wrong generated code and non-functional snippets in the UI.

This PR improves that by adding the relevant parts to the spec.

* style: use `OpenAPI` instead of `Swagger`

* refactor: make Set creation Java 8 compatible

* fix: add missing import

* [typescript*] drop support typescript below 4.0 (#12123)

* [typescript-axios] drop support typescript below 4.0 & update samples

* [typescript-axios] update package.json & package-lock.json

* [typescript-node] drop support typescript below 4.0 & update samples

* [typescript-nestjs] drop support typescript below 4.0 & update samples

* [typescript-redux-query] drop support typescript below 4.0 & update samples

* [typescript-aurelia] drop support typescript below 4.0 & update samples

* [typescript-jquery] drop support typescript below 4.0 & update samples

* [typescript] drop support typescript below 4.0 & update samples

* Upgrade haskell-servant to latest LTS (#12092)

* [C++][Qt] update petstore to 3.0 spec (#12124)

* test update sampels

* update samples

* add file

* update readme with onesignal (#12126)

* [typescript-fetch] Removed functions that are unused when withoutRuntime is true. (#12101)

* [typescript-fetch] remove unused function when withoutRuntimeCheks option to true

* [typescript-fetch] update samples

* [kotlin][client] fix encoding of individual parts of a multipart request (#11911)

* [kotlin][client] fix encoding (and Content-Type headers) of individual parts of a multipart request

* [kotlin][client] fix incorrect handling of binary downloads

* [kotlin][client] update samples

* [python-experimental] Allow response media types to omit schema (#12135)

* Adds issue spec file and attemts to generate code from it

* Adds missing schema definitions

* Skips fromProperty invocation if the passed in schema is none in getContent

* Makes MediaType.schema optional

* Adds checking that the content type is in self.content

* Sets ApiResponse body type as Unset if there is no schema for it

* Handles schema = None case

* Adds endpoint without response schema

* Reverts version files

* Adds test_response_without_schema

* improve errorObjectType to avoid regression (#12131)

* [php-slim4] Add monolog package as default logger (#12137)

* Add monolog to templates

* Remove default values from DI\get helper

It turned out \DI\get expects only single argument, current method call
doesn't throw any errors but it should be corrected anyway.

* Refresh samples

* Bump async from 2.6.3 to 2.6.4 in /website (#12148)

Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4.
- [Release notes](https://github.com/caolan/async/releases)
- [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md)
- [Commits](caolan/async@v2.6.3...v2.6.4)

---
updated-dependencies:
- dependency-name: async
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* do not put the invalid value of the enum to a JSON structure (#12133)

* [Java] Ignore return value for Java file assert classes (#12145)

* Add @mkusaka to TS technical committee (#12150)

* Fix documentation for users of AbstractJavaJAXRSServerCodegen (#12142)

The default for `sourceFolder` in the documentation was wrong.

* [Dart][client] Adjust toJson method to use '_json' instead of 'json' to avoid shadowing fields named 'json' (#12127) (#12138)

* [Dart][client] Updated dart samples

Co-authored-by: 0xnf <0xnf>

* [dart] Support/Fix sourceFolder parameter and docs (#12113)

* [dart] Support/Fix sourceFolder parameter and docs

* allow passing the sourceFolder to generators
* not sure how this translates to the dart2 generator due to the `part` files
* fix doc generation not showing default value of CLI options

* [dart] Delete old generator doc files

* [Ruby] Add support for faraday 2.x (#12112)

* [ruby] Add faraday 2.x support

* Remove redundant params_encoder config

* Memoize Faraday connection and refactor

* [Java/Spring] all-of and one-of Improvements and Fixes (was #12075) (#12089)

* Fix Bug in OneOfImplementorAdditionalData pulling in wrong vars to one-of-implementors.
Support parentVars in order to support fluent setter with inherited properties.

Squashed commit of the following:

commit f945c943777a1a496d7de8fc0a188842d9efb1ac
Author: Lars Uffmann <[email protected]>
Date:   Thu Apr 7 18:22:54 2022 +0200

    Polishing

commit 23ce1d0ff1faff53e85ca4362f33660962aa6a92
Author: Lars Uffmann <[email protected]>
Date:   Thu Apr 7 17:15:28 2022 +0200

    Add JavaDoc

commit fee70fde5709afa67f3aabd4f48ba496df63a884
Author: Lars Uffmann <[email protected]>
Date:   Thu Apr 7 17:11:17 2022 +0200

    Add imports for inherited Properties

commit 29509aaac51750fbd33c00a57d32cac34cbcbb90
Author: Lars Uffmann <[email protected]>
Date:   Thu Apr 7 13:40:36 2022 +0200

    Generate Samples

commit 1d19d5465137d3af712f2fd3b4ae4474c58af15e
Author: Lars Uffmann <[email protected]>
Date:   Thu Apr 7 13:21:23 2022 +0200

    SpringCodegen: Support parentVars in order to support fluent setter with inherited properties.

commit 2217a77bb747d0b07ef17407a6b5dd5c624a2551
Author: Lars Uffmann <[email protected]>
Date:   Thu Apr 7 07:18:50 2022 +0200

    Add allVars to omit list in OneOfImplementorAdditionalData

commit 90499a3b0a187971bfe25deb6355c3444dcf89a7
Author: Lars Uffmann <[email protected]>
Date:   Wed Apr 6 16:40:23 2022 +0200

    Works exactly as needed for oneOf/allOf in Java/Spring

commit b6d496d772e0d0a8d87a3b8cdba8fd3ca4db7f3f
Author: Lars Uffmann <[email protected]>
Date:   Wed Apr 6 15:16:27 2022 +0200

    Debug Session: identify critical codep path

commit 85722360038107f15841d5acc448d93dee513a06
Author: Lars Uffmann <[email protected]>
Date:   Tue Apr 5 09:56:38 2022 +0200

    Add test case to reproduce issue.

commit 14acc5cd974bb5260f3751015558807e2eb1a8a1
Author: Lars Uffmann <[email protected]>
Date:   Tue Apr 5 06:57:30 2022 +0200

    Add config to reproduce the issue

* Adjust indentation.

Co-authored-by: Lars Uffmann <[email protected]>

* [REQ][Ruby] Ruby Allow Follow Redirect (#12047)

* Add `follow_location` option

Implementation of #10028

* regenerated clients

* set follow_location default to true

* Adds UUID to python-experimental (#12153)

* Adds UUID to python-exp, allows uuid models to be generated

* Adds test_UUIDString uuid model test

* Fixes uuid properties in python-exp, changes maps to object data type, adds uuid data type

* Adds maps data type back in

* Adds missing Null and AnyType definitions and adds them to python-experimental

* Generator docs updated, added missing uuid, null, anytype, and object

* Adds uuid support description

* Docs updated

* update url to travis ci

* Adds not to CodegenComposedSchemas and uses it in python-exp (#12146)

Updates docs

* [python-experimental] fixes bug where some singleton representations raised a RecursionError  (#12157)

* Adds issue components and endpoint

* Regenerates samples

* Fixes singleton repr, removes issue components and endpoint

* Removes unused endpoint

* Reverts file

* Adds tests of enum, boolean, and none representations

* Uses super repr for singletons that ere not none, true, or false

* [swift5] Abstract away URLSession (#11651) (#12110)

* [Java][OkHTTP] fix empty request body handling (#12172)

* Better inline model resolver to handle inline schema in array item (#12104)

* better support of inline schema in array item

* update tests

* update samples

* regenerate samples

* fix allof naming, remove files

* add files

* update samples

* update readme

* fix tests

* update samples

* update samples

* add new files

* update test spec

* add back tests

* remove unused files

* comment out python test

* update js test using own spec

* remove files

* remove unused files

* remove files

* remove unused files

* better handling of allOf with a single type

* comment out go test

* remove test_all_of_with_single_ref_single_ref_type.py

* fix inline resolver, uncomment go test

* [Inline model resolver] minor enhancements/refactoring (#12175)

* better code format

* better code format, minor refactor

* [python-experimental] Fixes enum is comparison (#12176)

* Fixes enum is comparison

* Reverts file

* [php] make ObjectSerializer::toString actually return a string (#12158)

* update php samples

* Allow selection of MP REST API version for MicroProfile REST client g… (#12043)

* Allow selection of MP REST API version for MicroProfile REST client generation

* fix typo in pom.xml

* fix typo in pom.xml, update samples

* add exception when incorrect MP Rest Client version is chosen

* [Java][microprofile] update API test template to work with v3.0 (#12177)

* update microprofile api test to work with 3.0

* minor format change

* update samples

* Return type for Azure funcs (#12115)

* Azure func return type

* Changed to Task<IActionResult<T>

* Readme

* update doc

* Improvements to csharp-netcore-function generator (#12183)

* improvements to csharp-netcore-function generator

* update samples

* update doc, samples

* [java-micronaut] Support Optional for non-required properties (#12144)

The Micronaut generator by default adds the @nullable annotation to
non-required properties and allows using the Jackson JsonNullable
wrapper but it is not possible to use java.util.Optional as a wrapper
for optional properties.

This change adds support for using the Optional wrapper for non-required
properties.

* update java samples

* [typescript-fetch] allow initOverrides with async function (#12098)

* [typescript-fetch] allow initOverrides with async function

* [typescript-fetch] update samples

* [typescript-fetch] refactoring initFnction apply

* [typescript-fetch] update samples

* [typescript-fetch] refactoring create body function

* [typescript-fetch] update samples

* [typescript-fetch] make interface more flexible

* [typescript-fetch] update samples

* [typescript-fetch] support 2.x version of typescript & update samples

* [typescript-axios] update samples

* [typescript-fetch] refactor: add type alias

* [typescript-fetch] override with init params even if initOverrides is function

* [typescript-fetch] update samples

* [Wsdl] Adding cli-option for generating different versions of WSDL-files regarding Media type versioning (content negotiation) (#12206)

* add wsdl version generation

* add option to use specified operationId

* update samples

* update cli description

* [python-flask] Fix return type too strict (#12190)

Flask (and connexion by extension) allows the return type to be either
just the body, or the body & status code, or the body & status code &
headers.

This commit fixes the stated `rtype` to allow the latter two cases.

* update samples, docs

* Fixing bug in Kotlin Client with BigDecimal default value (#12213)

* Fix duplication of "Api" when structPrefix is set (#12128)

This fixes a minor duplication of the word "Api", which is already part
of the classname template parameter and doesn't need to be repeated when
structPrefix is set.

* add samples/client/petstore/kotlin-bigdecimal-default to kotlin ci tests

* remove spring-mvc samples (#12222)

* [Micronaut] Add option to describe response wrappers (#12186)

* Minor refactor for Micronaut generators

* Add support for security roles in micronaut server generator

* Micronaut Server Generator refactor the x-roles String variable

* Add support for Micronaut HttpResponse wrapper

* Generate samples

* Optimize the usage of context-path for Micronaut server

* Emit default values for aspnetcore 3 value types (#11280)

Fixes #10772 for aspnetcore 3+

This allows numbers to be set to zero, and booleans to be set to false.
It may make sense to port this fix to the other C# generators,
though it was partially fixed (for booleans only) in the netcore client in PR9042.

* update samples

Co-authored-by: Justin Black <[email protected]>
Co-authored-by: Peter Leibiger <[email protected]>
Co-authored-by: mkusaka <[email protected]>
Co-authored-by: William Cheng <[email protected]>
Co-authored-by: Florian Greinacher <[email protected]>
Co-authored-by: Tom Bärwinkel <[email protected]>
Co-authored-by: Anton Koscejev <[email protected]>
Co-authored-by: Yuriy Belenko <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hui Yu <[email protected]>
Co-authored-by: Oleh Kurpiak <[email protected]>
Co-authored-by: Eric Wolf <[email protected]>
Co-authored-by: 0xNF <[email protected]>
Co-authored-by: Yohei Kitamura <[email protected]>
Co-authored-by: cachescrubber <[email protected]>
Co-authored-by: Lars Uffmann <[email protected]>
Co-authored-by: Connor Moore <[email protected]>
Co-authored-by: leszek-s <[email protected]>
Co-authored-by: fengelniederhammer <[email protected]>
Co-authored-by: Andrii Serkes <[email protected]>
Co-authored-by: Abrhm7786 <[email protected]>
Co-authored-by: Auke Schrijnen <[email protected]>
Co-authored-by: adessoDpd <[email protected]>
Co-authored-by: Oliver Ford <[email protected]>
Co-authored-by: Johan Sjöblom <[email protected]>
Co-authored-by: Noah Fontes <[email protected]>
Co-authored-by: Andriy Dmytruk <[email protected]>
Co-authored-by: matt beary <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants