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

Add common headers for Platform, SDK version, Source app #455

Merged
merged 9 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions Gravatar.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ Pod::Spec.new do |s|
s.ios.deployment_target = ios_deployment_target

s.source_files = 'Sources/Gravatar/**/*.swift'
s.resource_bundles = {
'Gravatar' => ['Sources/Gravatar/Resources/*.plist']
}

# Using the `package` access level for types requires us to pass `-package-name`
# as a swift flag, with the same name for each module/pod
Expand Down
1 change: 1 addition & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ let package = Package(
// Targets can depend on other targets in this package and products from dependencies.
.target(
name: "Gravatar",
resources: [.process("Resources")],
swiftSettings: [
.enableExperimentalFeature("StrictConcurrency")
],
Expand Down
21 changes: 21 additions & 0 deletions Sources/Gravatar/Bundle+ResourceBundle.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import Foundation

#if !SWIFT_PACKAGE
private class BundleFinder: NSObject {}
extension Bundle {
static var module: Bundle {
let defaultBundle = Bundle(for: BundleFinder.self)
// If installed with CocoaPods, resources will be in Gravatar.bundle
// The name of the bundle "Gravatar.bundle" (without the .bundle file extension)
// needs to match the key in the respective Gravatar.podspec:
// `s.resource_bundles = { 'Gravatar' => ['Sources/Gravatar/Resources/*.plist'] }`
if let bundleURL = defaultBundle.resourceURL,
let resourceBundle = Bundle(url: bundleURL.appendingPathComponent("Gravatar.bundle"))
Copy link
Contributor

@andrewdmontgomery andrewdmontgomery Oct 3, 2024

Choose a reason for hiding this comment

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

Nice.

For those visiting this in the future:

The name of the bundle "Gravatar.bundle" (without the .bundle file extension) needs to match the key in the respective Gravatar.podspec:

 s.resource_bundles = {
    'Gravatar' => ['Sources/Gravatar/Resources/*.plist']
  }

We happen to be using the same name ("Gravatar") for the module and the pod, too. So it's easier to get right. But the actual dependency is on the name of the resource bundle we define in the podspec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for elaborating. This is valuable info so i added it in the code comment.

{
return resourceBundle
}
// Otherwise, the default bundle is used for resources
return defaultBundle
}
}
#endif
22 changes: 22 additions & 0 deletions Sources/Gravatar/BundleInfo.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import Foundation

enum BundleInfo {
public static var sdkVersion: String? {
getInfoValue(forKey: "CFBundleShortVersionString") as? String
}

public static var appName: String? {
Bundle.main.object(forInfoDictionaryKey: "CFBundleName") as? String
}

private static func getInfoValue(forKey key: String) -> Any? {
// Access the SDKInfo.plist using Bundle.module
guard let url = Bundle.module.url(forResource: "SDKInfo", withExtension: "plist"),
let data = try? Data(contentsOf: url),
let plist = try? PropertyListSerialization.propertyList(from: data, options: [], format: nil) as? [String: Any]
else {
return nil
}
return plist[key]
}
}
4 changes: 2 additions & 2 deletions Sources/Gravatar/Network/Services/AvatarService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public struct AvatarService: Sendable {
@discardableResult
@available(*, deprecated, renamed: "upload(_:accessToken:)")
public func upload(_ image: UIImage, email: Email, accessToken: String) async throws -> URLResponse {
try await imageUploader.uploadImage(image, accessToken: accessToken, additionalHTTPHeaders: [(name: "Client-Type", value: "ios")]).response
try await imageUploader.uploadImage(image, accessToken: accessToken, additionalHTTPHeaders: nil).response
}

/// Uploads an image to be used as the user's Gravatar profile image, and returns the `URLResponse` of the network tasks asynchronously. Throws
Expand All @@ -59,7 +59,7 @@ public struct AvatarService: Sendable {
@discardableResult
public func upload(_ image: UIImage, accessToken: String) async throws -> Avatar {
do {
let (data, _) = try await imageUploader.uploadImage(image, accessToken: accessToken, additionalHTTPHeaders: [(name: "Client-Type", value: "ios")])
let (data, _) = try await imageUploader.uploadImage(image, accessToken: accessToken, additionalHTTPHeaders: nil)
return try data.decode()
} catch let error as ImageUploadError {
throw error
Expand Down
3 changes: 3 additions & 0 deletions Sources/Gravatar/Network/Services/URLSessionHTTPClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ struct URLSessionHTTPClient: HTTPClient {
let configuration = URLSessionConfiguration.default
configuration.httpAdditionalHeaders = [
"Accept": "application/json",
"X-Platform": "ios",
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me of a separate question: do we want to track iOS/iPadOS? If so, I think we'd still treat them both as X-Platform ios, and add a separate header for device form factor. That way both platforms could record this.

We could add a separate header like "X-Device-Type: tablet".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now we don't want to track iOS/iPadOS and yes i agree that would be a separate header if needed.

"X-SDK-Version": BundleInfo.sdkVersion ?? "",
"X-Source": BundleInfo.appName ?? "",
]
self.urlSession = urlSession ?? URLSession(configuration: configuration)
}
Expand Down
8 changes: 8 additions & 0 deletions Sources/Gravatar/Resources/SDKInfo.plist
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>CFBundleShortVersionString</key>
<string>2.1.1</string>
</dict>
</plist>
1 change: 0 additions & 1 deletion Tests/GravatarTests/AvatarServiceTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ final class AvatarServiceTests: XCTestCase {
XCTAssertTrue(request?.value(forHTTPHeaderField: "Authorization")?.hasPrefix("Bearer ") ?? false)
XCTAssertNotNil(request?.value(forHTTPHeaderField: "Content-Type"))
XCTAssertTrue(request?.value(forHTTPHeaderField: "Content-Type")?.hasPrefix("multipart/form-data; boundary=") ?? false)
XCTAssertTrue(request?.value(forHTTPHeaderField: "Client-Type") == "ios")
}

func testUploadImageError() async throws {
Expand Down