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

SDKS-3458 Swift6 adoption + concurrency check #6

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/build-and-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [macos-14, macos-14-large]
os: [macos-15, macos-15-large]
runs-on: ${{ matrix.os }}
timeout-minutes: 20

Expand All @@ -29,17 +29,17 @@ jobs:
run: echo "CHIP_TYPE=$(uname -m)" >> $GITHUB_ENV

# Set target Xcode version. For more details and options see:
# https://github.com/actions/virtual-environments/blob/main/images/macos/macos-14-Readme.md
# https://github.com/actions/virtual-environments/blob/main/images/macos/macos-15-Readme.md
- name: Select Xcode
run: sudo xcode-select -switch /Applications/Xcode_15.4.app && /usr/bin/xcodebuild -version
run: sudo xcode-select -switch /Applications/Xcode_16.1.0.app && /usr/bin/xcodebuild -version

# Run all tests
- name: Run tests
run: xcodebuild test -scheme PingTestHost -workspace SampleApps/Ping.xcworkspace -configuration Debug -destination 'platform=iOS Simulator,name=iPhone 15 Pro Max,OS=17.5' -derivedDataPath DerivedData -enableCodeCoverage YES -resultBundlePath TestResults | xcpretty && exit ${PIPESTATUS[0]}
run: xcodebuild test -scheme PingTestHost -workspace SampleApps/Ping.xcworkspace -configuration Debug -destination 'platform=iOS Simulator,name=iPhone 16 Pro Max,OS=18.1' -derivedDataPath DerivedData -enableCodeCoverage YES -resultBundlePath TestResults | xcpretty && exit ${PIPESTATUS[0]}

# Publish test results
- name: Publish test results
uses: kishikawakatsumi/xcresulttool@v1
uses: slidoapp/xcresulttool@v3.1.0
with:
title: "Test Results ${{ matrix.os }} - ${{ env.CHIP_TYPE }}"
path: TestResults.xcresult
Expand Down
10 changes: 8 additions & 2 deletions PingDavinci/PingDavinci.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,8 @@
SDKROOT = iphoneos;
SWIFT_ACTIVE_COMPILATION_CONDITIONS = "DEBUG $(inherited)";
SWIFT_OPTIMIZATION_LEVEL = "-Onone";
SWIFT_STRICT_CONCURRENCY = complete;
SWIFT_VERSION = 6.0;
VERSIONING_SYSTEM = "apple-generic";
VERSION_INFO_PREFIX = "";
};
Expand Down Expand Up @@ -497,6 +499,8 @@
MTL_FAST_MATH = YES;
SDKROOT = iphoneos;
SWIFT_COMPILATION_MODE = wholemodule;
SWIFT_STRICT_CONCURRENCY = complete;
SWIFT_VERSION = 6.0;
VALIDATE_PRODUCT = YES;
VERSIONING_SYSTEM = "apple-generic";
VERSION_INFO_PREFIX = "";
Expand Down Expand Up @@ -533,7 +537,8 @@
SKIP_INSTALL = YES;
SWIFT_EMIT_LOC_STRINGS = YES;
SWIFT_OPTIMIZATION_LEVEL = "-Onone";
SWIFT_VERSION = 5.0;
SWIFT_STRICT_CONCURRENCY = complete;
SWIFT_VERSION = 6.0;
TARGETED_DEVICE_FAMILY = "1,2";
};
name = Debug;
Expand Down Expand Up @@ -567,7 +572,8 @@
PRODUCT_NAME = "$(TARGET_NAME:c99extidentifier)";
SKIP_INSTALL = YES;
SWIFT_EMIT_LOC_STRINGS = YES;
SWIFT_VERSION = 5.0;
SWIFT_STRICT_CONCURRENCY = complete;
SWIFT_VERSION = 6.0;
TARGETED_DEVICE_FAMILY = "1,2";
};
name = Release;
Expand Down
4 changes: 2 additions & 2 deletions PingDavinci/PingDavinci/collector/Collector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ import PingOrchestrate
import Foundation

/// Protocol representing a Collector.
public protocol Collector: Action, Identifiable {
public protocol Collector: Action, Identifiable, Sendable {
var id: UUID { get }
init(with json: [String: Any])
init(with json: Field)
}

extension ContinueNode {
Expand Down
74 changes: 37 additions & 37 deletions PingDavinci/PingDavinci/collector/CollectorFactory.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,42 +14,42 @@ import PingOrchestrate
/// The CollectorFactory singleton is responsible for creating and managing Collector instances.
/// It maintains a dictionary of collector creation functions, keyed by type.
/// It also provides functions to register new types of collectors and to create collectors from a JSON array.
final class CollectorFactory {
// A dictionary to hold the collector creation functions.
public var collectors: [String: any Collector.Type] = [:]

public static let shared = CollectorFactory()

init() {
register(type: Constants.TEXT, collector: TextCollector.self)
register(type: Constants.PASSWORD, collector: PasswordCollector.self)
register(type: Constants.SUBMIT_BUTTON, collector: SubmitCollector.self)
register(type: Constants.FLOW_BUTTON, collector: FlowCollector.self)
}

/// Registers a new type of Collector.
/// - Parameters:
/// - type: The type of the Collector.
/// - block: A function that creates a new instance of the Collector.
public func register(type: String, collector: any Collector.Type) {
collectors[type] = collector
}

/// Creates a list of Collector instances from an array of dictionaries.
/// Each dictionary should have a "type" field that matches a registered Collector type.
/// - Parameter array: The array of dictionaries to create the Collectors from.
/// - Returns: A list of Collector instances.
func collector(from array: [[String: Any]]) -> Collectors {
var list: [any Collector] = []
for item in array {
if let type = item[Constants.type] as? String, let collectorType = collectors[type] {
list.append(collectorType.init(with: item))
}
}
return list
}

func reset() {
collectors.removeAll()
public actor CollectorFactory {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a Singleton?

Copy link
Contributor Author

@jeyanthanperiyasamy jeyanthanperiyasamy Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, CollectorFactory.shared is the usage.

So customer will have to call like to add custom callback ..
await CollectorFactory.shared.register(type: collector:)

// A dictionary to hold the collector creation functions.
public var collectors: [String: any Collector.Type] = [:]

public static let shared = CollectorFactory()

init() {
collectors[Constants.TEXT] = TextCollector.self
collectors[Constants.PASSWORD] = PasswordCollector.self
collectors[Constants.SUBMIT_BUTTON] = SubmitCollector.self
collectors[Constants.FLOW_BUTTON] = FlowCollector.self
}

/// Registers a new type of Collector.
/// - Parameters:
/// - type: The type of the Collector.
/// - block: A function that creates a new instance of the Collector.
public func register(type: String, collector: any Collector.Type) {
collectors[type] = collector
}

/// Creates a list of Collector instances from an array of dictionaries.
/// Each dictionary should have a "type" field that matches a registered Collector type.
/// - Parameter array: The array of dictionaries to create the Collectors from.
/// - Returns: A list of Collector instances.
func collector(from array: [Field]) -> Collectors {
var list: [any Collector] = []
for item in array {
if let collectorType = collectors[item.type] {
list.append(collectorType.init(with: item))
}
}
return list
}

func reset() {
collectors.removeAll()
}
}
53 changes: 46 additions & 7 deletions PingDavinci/PingDavinci/collector/FieldCollector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,59 @@

import Foundation


/// Abstract class representing a field collector.
/// - property key: The key of the field collector.
/// - property label The label of the field collector.
/// - property value The value of the field collector. It's open for modification.
open class FieldCollector: Collector {
public var key: String = ""
public var label: String = ""
public var value: String = ""
open class FieldCollector: Collector, @unchecked Sendable {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you handle concurrent update of attributes, why define as unchecked?

Copy link
Contributor Author

@jeyanthanperiyasamy jeyanthanperiyasamy Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Swift 6, this code won't compile as-is, because the compiler will error:
"Non-final class 'FieldCollector' cannot conform to 'Sendable'".

To resolve this, we need to instruct the compiler to skip the compatibility check by explicitly marking the conformance as unchecked. This can be done using @unchecked Sendable,


// Private queue for thread-safe access
private let syncQueue = DispatchQueue(label: "com.fieldCollector.syncQueue", attributes: .concurrent)

// Private backing properties to store data
private var _key: String = ""
private var _label: String = ""
private var _value: String = ""

// Public computed properties for thread-safe access
public var key: String {
get {
return syncQueue.sync { _key }
}
set {
syncQueue.async(flags: .barrier) { self._key = newValue }
}
}

public var label: String {
get {
return syncQueue.sync { _label }
}
set {
syncQueue.async(flags: .barrier) { self._label = newValue }
}
}

public var value: String {
get {
return syncQueue.sync { _value }
}
set {
syncQueue.async(flags: .barrier) { self._value = newValue }
}
}

public let id = UUID()

// Default initializer
public init() {}

required public init(with json: [String: Any]) {
key = json[Constants.key] as? String ?? ""
label = json[Constants.label] as? String ?? ""
// Initializer with a JSON field
required public init(with json: Field) {
syncQueue.async(flags: .barrier) {
self._key = json.key
self._label = json.label
}
}
}
2 changes: 1 addition & 1 deletion PingDavinci/PingDavinci/collector/FlowCollector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@ import Foundation
/// Class representing a FlowCollector.
/// This class inherits from the FieldCollector class and implements the Collector protocol.
/// It is used to collect data in a flow.
public class FlowCollector: FieldCollector {}
public class FlowCollector: FieldCollector, @unchecked Sendable {}
52 changes: 38 additions & 14 deletions PingDavinci/PingDavinci/collector/Form.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,44 @@ import Foundation

/// Class that handles the parsing and JSON representation of collectors.
/// This class provides functions to parse a JSON object into a list of collectors and to represent a list of collectors as a JSON object.
class Form {

/// Parses a JSON object into a list of collectors.
/// This function takes a JSON object and extracts the "form" field. It then iterates over the "fields" array in the "components" object,
/// parsing each field into a collector and adding it to a list.
/// - Parameter json :The JSON object to parse.
/// - Returns: A list of collectors parsed from the JSON object.
static func parse(json: [String: Any]) -> Collectors {
var collectors = Collectors()
if let form = json[Constants.form] as? [String: Any],
let components = form[Constants.components] as? [String: Any],
let fields = components[Constants.fields] as? [[String: Any]] {
collectors = CollectorFactory().collector(from: fields)
actor Form {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not mutable state in Form, any reason we need to define it as actor?

Copy link
Contributor Author

@jeyanthanperiyasamy jeyanthanperiyasamy Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Form can be a struct ... we generally reduce using classes. For this scenario actor is not required. i will change this.


/// Parses a JSON object into a list of collectors.
/// This function takes a JSON object and extracts the "form" field. It then iterates over the "fields" array in the "components" object,
/// parsing each field into a collector and adding it to a list.
/// - Parameter json :The JSON object to parse.
/// - Returns: A list of collectors parsed from the JSON object.
static func parse(json: [String: Any]) async -> Collectors {
var collectors = Collectors()
if let form = json[Constants.form] as? [String: Any],
let components = form[Constants.components] as? [String: Any],
let fields = components[Constants.fields] as? [[String: Any]] {

let factory = CollectorFactory.shared

let fields: [Field] = fields.compactMap { fieldDict in
if let type = fieldDict["type"] as? String {
let value = fieldDict["value"] as? String ?? ""
let key = fieldDict[Constants.key] as? String ?? ""
let label = fieldDict[Constants.label] as? String ?? ""

return Field(type: type, value: value, key: key, label: label)
}
return collectors
return nil
}

collectors = await factory.collector(from: fields)
return collectors

}
return collectors
}
}


public struct Field: Sendable {
let type: String
let value: String
let key: String
let label: String
}
2 changes: 1 addition & 1 deletion PingDavinci/PingDavinci/collector/PasswordCollector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import PingOrchestrate
/// Class representing a PasswordCollector.
/// This class inherits from the FieldCollector class and implements the Closeable and Collector protocols.
/// It is used to collect password data.
public class PasswordCollector: FieldCollector, Closeable {
public class PasswordCollector: FieldCollector, Closeable, @unchecked Sendable {
public var clearPassword: Bool = true

/// Overrides the close function from the Closeable protocol.
Expand Down
2 changes: 1 addition & 1 deletion PingDavinci/PingDavinci/collector/SubmitCollector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@ import Foundation
/// Class representing a TextCollector.
/// This class inherits from the FieldCollector class and implements the Collector protocol.
/// `SubmitCollector` is responsible for collecting and managing submission fields.
public class SubmitCollector: FieldCollector {}
public class SubmitCollector: FieldCollector, @unchecked Sendable {}
2 changes: 1 addition & 1 deletion PingDavinci/PingDavinci/collector/TextCollector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@ import Foundation
/// Class representing a TextCollector.
/// This class inherits from the FieldCollector class and implements the Collector protocol.
/// It is used to collect text data.
public class TextCollector: FieldCollector {}
public class TextCollector: FieldCollector, @unchecked Sendable {}
2 changes: 1 addition & 1 deletion PingDavinci/PingDavinci/module/Connector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ extension ContinueNode {
///- property workflow: The Workflow of the ContinueNode.
///- property input: The input JsonObject of the ContinueNode.
///- property collectors: The collectors of the ContinueNode.
class DaVinciConnector: ContinueNode {
class DaVinciConnector: ContinueNode, @unchecked Sendable {

init(context: FlowContext, workflow: Workflow, input: [String: Any], collectors: Collectors) {
super.init(context: context, workflow: workflow, input: input, actions: collectors)
Expand Down
2 changes: 1 addition & 1 deletion PingDavinci/PingDavinci/module/Oidc.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import Foundation
import PingOidc
import PingOrchestrate

public class OidcModule {
public actor OidcModule {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no state in OidcModule, why define it as actor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by default static property in classes are not thread safe. so compiler will throw error

Static property 'config' is not concurrency-safe because non-'Sendable' type 'Module<OidcClientConfig>' may have shared mutable state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me think about this in another way


public init() {}

Expand Down
13 changes: 8 additions & 5 deletions PingDavinci/PingDavinci/module/Transform.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import PingOidc
import PingOrchestrate

/// Module for transforming the response from DaVinci to `Node`.
public class NodeTransformModule {
public actor NodeTransformModule {

public static let config: Module<Void> = Module.of(setup: { setup in
setup.transform { flowContext, response in
Expand Down Expand Up @@ -55,7 +55,7 @@ public class NodeTransformModule {
return FailureNode(cause: ApiError.error(status, json, body))
}

return transform(context: flowContext, workflow: setup.workflow, json: json)
return await transform(context: flowContext, workflow: setup.workflow, json: json)
}

// 5XX errors are treated as unrecoverable failures
Expand All @@ -64,15 +64,16 @@ public class NodeTransformModule {

})

private static func transform(context: FlowContext, workflow: Workflow, json: [String: Any]) -> Node {
private static func transform(context: FlowContext, workflow: Workflow, json: [String: Any]) async -> Node {
// If authorizeResponse is present, return success
if let _ = json[Constants.authorizeResponse] as? [String: Any] {
return SuccessNode(input: json, session: SessionResponse(json: json))
}

var collectors: Collectors = []
if let _ = json[Constants.form] {
collectors.append(contentsOf: Form.parse(json: json))
let form = await Form.parse(json: json)
collectors.append(contentsOf: form)
}

return DaVinciConnector(context: context, workflow: workflow, input: json, collectors: collectors)
Expand All @@ -92,6 +93,8 @@ struct SessionResponse: Session {
}
}

public enum ApiError: Error {

public enum ApiError: Error, @unchecked Sendable {
case error(Int, [String: Any], String)
}

Loading
Loading