search

AvdLee / appstoreconnect-swift-sdk

The Swift SDK to work with the App Store Connect API from Apple.
Other
1.48k stars 198 forks source link

change APIProvider to be an actor instead of a class #242

Closed nickasd closed 1 year ago

nickasd commented 1 year ago

Solves #238.

nickasd commented 1 year ago

I just followed your instructions on how to run the tests and noticed that I still had to change the tests.

I don't know if I'm doing something wrong, but running bundle exec fastlane test first gave an error

bundler: command not found: fastlane
Install missing gem executables with `bundle install`

But running bundle install also wasn't sufficient. I had to add a line to Gemfile:

gem "fastlane"

Then I was able to run the tests. But they fail in the Terminal:

xctest (95287) encountered an error (Failed to create a bundle instance representing 'appstoreconnect-swift-sdk/build/derived_data/Build/Products/Debug-iphonesimulator/AppStoreConnect-Swift-SDK-Tests.xctest'. Check that the bundle exists on disk. If you believe this error represents a bug, please attach the result bundle at appstoreconnect-swift-sdk/build/reports/AppStoreConnect-Swift-SDK.xcresult)

In fact, when opening appstoreconnect-swift-sdk/.swiftpm/xcode/package.xcworkspace in Xcode and running the tests with Command-U, I also get an error:

Logic Testing Unavailable Logic Testing on iOS devices is not supported. You can run logic tests on the Simulator.

I had to change the target to My Mac and then the tests all passed. But I don't know what I have to do in order to be able to run the tests in the Terminal.

SwiftLeeBot commented 1 year ago
Warnings
:warning: 'Prices' is deprecated: Deprecated
:warning: 'AvailableTerritories' is deprecated: Deprecated
:warning: 'InAppPurchases' is deprecated: Deprecated
:warning: 'Prices' is deprecated: Deprecated
:warning: 'AvailableTerritories' is deprecated: Deprecated
:warning: 'InAppPurchases' is deprecated: Deprecated
:warning: 'Builds' is deprecated: Deprecated
:warning: 'Builds' is deprecated: Deprecated
:warning: 'AppPrice' is deprecated: Deprecated
:warning: 'AgeRatingDeclaration' is deprecated: Deprecated
:warning: 'AgeRatingDeclaration' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppStoreVersionSubmission' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppStoreVersionSubmission' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'Prices' is deprecated: Deprecated
:warning: 'AvailableTerritories' is deprecated: Deprecated
:warning: 'Prices' is deprecated: Deprecated
:warning: 'AvailableTerritories' is deprecated: Deprecated
:warning: 'AppPrice' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'Builds' is deprecated: Deprecated
:warning: 'AppStoreVersionSubmission' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppStoreVersionSubmission' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'Prices' is deprecated: Deprecated
:warning: 'AvailableTerritories' is deprecated: Deprecated
:warning: 'AppPrice' is deprecated: Deprecated
:warning: 'AgeRatingDeclaration' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppPrice' is deprecated: Deprecated
:warning: Left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used
:warning: 'Prices' is deprecated: Deprecated
:warning: 'AvailableTerritories' is deprecated: Deprecated
:warning: 'InAppPurchases' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
:warning: 'AppStoreVersionExperiment' is deprecated: Deprecated
Messages
:book: AppStoreConnect-Swift-SDK-Tests: Executed 17 tests (0 failed, 0 retried, 0 skipped) in 0.307 seconds
:book: View more details on Bitrise

Code Coverage Report

Name Coverage

SwiftLint found issues

Severity File Reason
Warning APIProviderTests.swift:13 Colons should be next to the identifier when specifying a type and next to the key in dictionary literals (colon)
Warning APIProviderTests.swift:43 Method 'setUp()' should call to super function (overridden_super_call)
Warning APIProviderTests.swift:9 Imports should be sorted (sorted_imports)
Warning APIProvider.swift:76 Use shorthand syntax for optional binding (shorthand_optional_binding)
Warning APIProvider.swift:138 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:244 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:274 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:295 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:312 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:322 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:338 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:351 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:362 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:372 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:380 Lines should not have trailing whitespace (trailing_whitespace)
Warning APIProvider.swift:106 Parentheses are not needed when declaring closure arguments (unneeded_parentheses_in_closure_argument)

Generated by :no_entry_sign: Danger Swift against f171bc2ff5b599ecdb5f1812415ceed113dfad2d

nickasd commented 1 year ago

Thanks a lot for your contribution! Unfortunately, I don't think this will solve the issue. It will make it impossible to run requests in parallel, which is a decrease in performance.

Thanks for checking. Are you sure that requests cannot run in parallel? I just tried changing ApiProvider.swift by replacing this part

public func request<T>(_ endpoint: Request<T>) async throws -> T where T: Decodable {
    try await withCheckedThrowingContinuation { continuation in
        request(endpoint, completion: continuation.resume(with:))
    }
}

with this

public func request<T>(_ endpoint: Request<T>) async throws -> T where T: Decodable {
    return try await withCheckedThrowingContinuation { continuation in
        print("start \(endpoint.path)")
        request(endpoint) { result in
            print("end \(endpoint.path)")
            continuation.resume(with: result)
        }
    }
}

When running my app, which loads different resources at the same time, I get this output:

start /v1/apps/1516126920/appStoreVersions
end /v1/apps/1516126920/appStoreVersions
start /v1/appStoreVersions/a10abbf3-d8c1-41c5-b8d6-5584d6233aee/appStoreVersionLocalizations
start /v1/appStoreVersions/b7f6beb9-b1eb-4a4e-be81-1b8cbef43a27/appStoreVersionLocalizations
end /v1/appStoreVersions/b7f6beb9-b1eb-4a4e-be81-1b8cbef43a27/appStoreVersionLocalizations
start /v1/appStoreVersionLocalizations/d9b5cd20-5985-4173-a539-c5fd30486c9c/appPreviewSets
start /v1/appStoreVersionLocalizations/5d785f0d-3a6b-4239-b92c-0e6fc2c6b385/appPreviewSets
start /v1/appStoreVersionLocalizations/ddc0db09-6a40-4d1b-ad36-80d3b57b1333/appPreviewSets
start /v1/appStoreVersionLocalizations/52e7c3e9-0189-4aec-a3ca-02f9a2b375eb/appPreviewSets
start /v1/appStoreVersionLocalizations/6d90d4e0-2bc6-4403-ae8d-cc80850d87e4/appPreviewSets
end /v1/appStoreVersions/a10abbf3-d8c1-41c5-b8d6-5584d6233aee/appStoreVersionLocalizations
start /v1/appStoreVersionLocalizations/15e208e5-451a-4e88-9e1d-09cfb68bc416/appPreviewSets
start /v1/appStoreVersionLocalizations/1aa0bcda-4d55-4c87-8b5a-8d8582a1578d/appPreviewSets
start /v1/appStoreVersionLocalizations/54d2e544-6b35-47f5-b49a-a12694dd0a95/appPreviewSets
start /v1/appStoreVersionLocalizations/f24c988e-0551-4bfb-acee-7bc0968a40e7/appPreviewSets
start /v1/appStoreVersionLocalizations/326f5d17-97de-4a95-9b81-be06499dda6e/appPreviewSets
end /v1/appStoreVersionLocalizations/ddc0db09-6a40-4d1b-ad36-80d3b57b1333/appPreviewSets
end /v1/appStoreVersionLocalizations/6d90d4e0-2bc6-4403-ae8d-cc80850d87e4/appPreviewSets
end /v1/appStoreVersionLocalizations/52e7c3e9-0189-4aec-a3ca-02f9a2b375eb/appPreviewSets
end /v1/appStoreVersionLocalizations/15e208e5-451a-4e88-9e1d-09cfb68bc416/appPreviewSets
end /v1/appStoreVersionLocalizations/54d2e544-6b35-47f5-b49a-a12694dd0a95/appPreviewSets
end /v1/appStoreVersionLocalizations/d9b5cd20-5985-4173-a539-c5fd30486c9c/appPreviewSets
start /v1/appPreviewSets/2c2825e5-375f-4d8f-8664-0239f0552a87/appPreviews
end /v1/appStoreVersionLocalizations/5d785f0d-3a6b-4239-b92c-0e6fc2c6b385/appPreviewSets
end /v1/appStoreVersionLocalizations/f24c988e-0551-4bfb-acee-7bc0968a40e7/appPreviewSets
end /v1/appStoreVersionLocalizations/1aa0bcda-4d55-4c87-8b5a-8d8582a1578d/appPreviewSets
end /v1/appStoreVersionLocalizations/326f5d17-97de-4a95-9b81-be06499dda6e/appPreviewSets
end /v1/appPreviewSets/2c2825e5-375f-4d8f-8664-0239f0552a87/appPreviews

Isn't this a sign that requests run in parallel?

mathiasemil commented 1 year ago

Thanks a lot for your contribution! Unfortunately, I don't think this will solve the issue. It will make it impossible to run requests in parallel, which is a decrease in performance.

Thanks for checking. Are you sure that requests cannot run in parallel? I just tried changing ApiProvider.swift by replacing this part

public func request<T>(_ endpoint: Request<T>) async throws -> T where T: Decodable {
    try await withCheckedThrowingContinuation { continuation in
        request(endpoint, completion: continuation.resume(with:))
    }
}

with this

public func request<T>(_ endpoint: Request<T>) async throws -> T where T: Decodable {
    return try await withCheckedThrowingContinuation { continuation in
        print("start \(endpoint.path)")
        request(endpoint) { result in
            print("end \(endpoint.path)")
            continuation.resume(with: result)
        }
    }
}

When running my app, which loads different resources at the same time, I get this output:

start /v1/apps/1516126920/appStoreVersions
end /v1/apps/1516126920/appStoreVersions
start /v1/appStoreVersions/a10abbf3-d8c1-41c5-b8d6-5584d6233aee/appStoreVersionLocalizations
start /v1/appStoreVersions/b7f6beb9-b1eb-4a4e-be81-1b8cbef43a27/appStoreVersionLocalizations
end /v1/appStoreVersions/b7f6beb9-b1eb-4a4e-be81-1b8cbef43a27/appStoreVersionLocalizations
start /v1/appStoreVersionLocalizations/d9b5cd20-5985-4173-a539-c5fd30486c9c/appPreviewSets
start /v1/appStoreVersionLocalizations/5d785f0d-3a6b-4239-b92c-0e6fc2c6b385/appPreviewSets
start /v1/appStoreVersionLocalizations/ddc0db09-6a40-4d1b-ad36-80d3b57b1333/appPreviewSets
start /v1/appStoreVersionLocalizations/52e7c3e9-0189-4aec-a3ca-02f9a2b375eb/appPreviewSets
start /v1/appStoreVersionLocalizations/6d90d4e0-2bc6-4403-ae8d-cc80850d87e4/appPreviewSets
end /v1/appStoreVersions/a10abbf3-d8c1-41c5-b8d6-5584d6233aee/appStoreVersionLocalizations
start /v1/appStoreVersionLocalizations/15e208e5-451a-4e88-9e1d-09cfb68bc416/appPreviewSets
start /v1/appStoreVersionLocalizations/1aa0bcda-4d55-4c87-8b5a-8d8582a1578d/appPreviewSets
start /v1/appStoreVersionLocalizations/54d2e544-6b35-47f5-b49a-a12694dd0a95/appPreviewSets
start /v1/appStoreVersionLocalizations/f24c988e-0551-4bfb-acee-7bc0968a40e7/appPreviewSets
start /v1/appStoreVersionLocalizations/326f5d17-97de-4a95-9b81-be06499dda6e/appPreviewSets
end /v1/appStoreVersionLocalizations/ddc0db09-6a40-4d1b-ad36-80d3b57b1333/appPreviewSets
end /v1/appStoreVersionLocalizations/6d90d4e0-2bc6-4403-ae8d-cc80850d87e4/appPreviewSets
end /v1/appStoreVersionLocalizations/52e7c3e9-0189-4aec-a3ca-02f9a2b375eb/appPreviewSets
end /v1/appStoreVersionLocalizations/15e208e5-451a-4e88-9e1d-09cfb68bc416/appPreviewSets
end /v1/appStoreVersionLocalizations/54d2e544-6b35-47f5-b49a-a12694dd0a95/appPreviewSets
end /v1/appStoreVersionLocalizations/d9b5cd20-5985-4173-a539-c5fd30486c9c/appPreviewSets
start /v1/appPreviewSets/2c2825e5-375f-4d8f-8664-0239f0552a87/appPreviews
end /v1/appStoreVersionLocalizations/5d785f0d-3a6b-4239-b92c-0e6fc2c6b385/appPreviewSets
end /v1/appStoreVersionLocalizations/f24c988e-0551-4bfb-acee-7bc0968a40e7/appPreviewSets
end /v1/appStoreVersionLocalizations/1aa0bcda-4d55-4c87-8b5a-8d8582a1578d/appPreviewSets
end /v1/appStoreVersionLocalizations/326f5d17-97de-4a95-9b81-be06499dda6e/appPreviewSets
end /v1/appPreviewSets/2c2825e5-375f-4d8f-8664-0239f0552a87/appPreviews

Isn't this a sign that requests run in parallel?

Hi @nickasd 👋🏼 I'll chime in here. If ApiProvider is an actor and you make all the requests from a single ApiProvider instance I will expect them to be executed sequentially. Even if that's not the case I will prefer ApiProvider to not be an actor to continue support for non-async calls.

To follow @AvdLee's suggestion I think we can fix the underlying issue in JWTRequestsAuthenticator with the following changes:

private let dispatchQueue = DispatchQueue(label: "JWTRequestsAuthenticator", attributes: .concurrent)

private func createToken() throws -> JWT.Token {
        try dispatchQueue.sync(flags: .barrier) {
            if let cachedToken = cachedToken, !cachedToken.isExpired {
                return cachedToken
            }

            let token = try jwtCreator.signedToken(using: apiConfiguration.privateKey)
            cachedToken = token
            return token
        }
}

I can make a PR to make this easier to read.

Let me know what you think 🙏🏼

nickasd commented 1 year ago

Hey @mathiasemil, that should work as well. But why use a concurrent queue if you only submit barriers to it, and not use a standard serial queue?

I was just thinking, since what we want to protect here is concurrent read and write to the cachedToken property, can't we instead assign cachedToken once at the beginning, e.g. in apiConfiguration.didSet?

mathiasemil commented 1 year ago

Hey @mathiasemil, that should work as well. But why use a concurrent queue if you only submit barriers to it, and not use a standard serial queue?

I was just thinking, since what we want to protect here is concurrent read and write to the cachedToken property, can't we instead assign cachedToken once at the beginning, e.g. in apiConfiguration.didSet?

I guess in this case there is no reason to not use a serial queue instead. So that sounds like a better approach 👍🏼

nickasd commented 1 year ago

I just realized now that it also checks for !cachedToken.isExpired, so obviously the token cannot be created once at the beginning.

I'll update the PR.

nickasd commented 1 year ago

Continued in #246.