Replace Device struct with the OpenAPI generated model#4161
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthrough
ChangesDevice model migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SDK Performance
|
Generated by 🚫 Danger |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/StreamChatTests/Database/DTOs/DeviceDTO_Tests.swift (1)
45-75: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGood round-trip coverage; consider adding a
false/nil-boolean variant.Solid test verifying all persisted
Devicefields. The boolean fields (disabled,voip) are only exercised withtrue; givenDeviceDTOwraps them viaNSNumber(value:), an explicitfalsecase would guard against future regressions wherefalsecould be mistaken for "unset".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/StreamChatTests/Database/DTOs/DeviceDTO_Tests.swift` around lines 45 - 75, Add a round-trip test for the DeviceDTO boolean fields using a false/nil-style case, because the current DeviceDTO_Tests coverage only verifies true values for disabled and voip. Update or add a test alongside test_deviceAllFields_areStoredAndLoadedFromDB to persist a Device with disabled and voip set to false, then load it back through currentUser?.asModel() and assert the values remain false; this ensures DeviceDTO’s NSNumber(value:) mapping does not accidentally treat false as unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/StreamChatTests/Database/DTOs/DeviceDTO_Tests.swift`:
- Around line 45-75: Add a round-trip test for the DeviceDTO boolean fields
using a false/nil-style case, because the current DeviceDTO_Tests coverage only
verifies true values for disabled and voip. Update or add a test alongside
test_deviceAllFields_areStoredAndLoadedFromDB to persist a Device with disabled
and voip set to false, then load it back through currentUser?.asModel() and
assert the values remain false; this ensures DeviceDTO’s NSNumber(value:)
mapping does not accidentally treat false as unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05abb598-346b-45ae-9d5d-bffcf7d7f51b
⛔ Files ignored due to path filters (2)
Sources/StreamChat/Generated/OpenAPI/models/Device.swiftis excluded by!**/generated/**Sources/StreamChat/Generated/OpenAPI/models/ListDevicesResponse.swiftis excluded by!**/generated/**
📒 Files selected for processing (23)
CHANGELOG.mdScripts/openapi_generate.shSources/StreamChat/APIClient/Endpoints/Payloads/CurrentUserPayloads.swiftSources/StreamChat/APIClient/Endpoints/Payloads/DevicePayloads.swiftSources/StreamChat/Database/DTOs/CurrentUserDTO.swiftSources/StreamChat/Database/DTOs/DeviceDTO.swiftSources/StreamChat/Database/DatabaseSession.swiftSources/StreamChat/Database/StreamChatModel.xcdatamodeld/StreamChatModel.xcdatamodel/contentsSources/StreamChat/Models/Device+Extensions.swiftSources/StreamChat/Models/Device.swiftSources/StreamChat/Workers/CurrentUserUpdater.swiftStreamChat.xcodeproj/project.pbxprojTestTools/StreamChatTestTools/Fixtures/JSONs/Devices.jsonTestTools/StreamChatTestTools/Mocks/StreamChat/Database/DatabaseSession_Mock.swiftTestTools/StreamChatTestTools/TestData/DummyData/CurrentUserPayload.swiftTestTools/StreamChatTestTools/TestData/DummyData/Device+Dummy.swiftTestTools/StreamChatTestTools/TestData/DummyData/DevicePayloads.swiftTests/StreamChatTests/APIClient/Endpoints/Payloads/DevicePayloads_Tests.swiftTests/StreamChatTests/Database/DTOs/CurrentUserDTO_Tests.swiftTests/StreamChatTests/Database/DTOs/DeviceDTO_Tests.swiftTests/StreamChatTests/Database/DTOs/UserDTO_Tests.swiftTests/StreamChatTests/StateLayer/ConnectedUser_Tests.swiftTests/StreamChatTests/Workers/CurrentUserUpdater_Tests.swift
💤 Files with no reviewable changes (6)
- Tests/StreamChatTests/APIClient/Endpoints/Payloads/DevicePayloads_Tests.swift
- TestTools/StreamChatTestTools/Fixtures/JSONs/Devices.json
- Sources/StreamChat/APIClient/Endpoints/Payloads/DevicePayloads.swift
- Sources/StreamChat/Models/Device.swift
- TestTools/StreamChatTestTools/TestData/DummyData/DevicePayloads.swift
- StreamChat.xcodeproj/project.pbxproj
| class DeviceDTO: NSManagedObject { | ||
| @NSManaged var id: String | ||
| @NSManaged var createdAt: DBDate? | ||
| @NSManaged var disabled: NSNumber? |
There was a problem hiding this comment.
Represents Bool? in the generated model. Not much to do here if we wanna switch to OpenAPI model directly without any additional wrapper.
| extension Device: Equatable { | ||
| public static func == (lhs: Device, rhs: Device) -> Bool { | ||
| lhs.id == rhs.id | ||
| && lhs.createdAt == rhs.createdAt | ||
| && lhs.disabled == rhs.disabled | ||
| && lhs.disabledReason == rhs.disabledReason | ||
| && lhs.hardwareId == rhs.hardwareId | ||
| && lhs.pushProvider == rhs.pushProvider | ||
| && lhs.pushProviderName == rhs.pushProviderName | ||
| && lhs.userId == rhs.userId | ||
| && lhs.voip == rhs.voip | ||
| } | ||
| } |
There was a problem hiding this comment.
OpenAPI generator change is coming which allows me to generate this. Next PR can clean it up.
| public final class Device: Sendable, Codable, JSONEncodable { | ||
| /// Date/time of creation | ||
| let createdAt: Date | ||
| public let createdAt: Date |
There was a problem hiding this comment.
Note! createdAt used be an optional value in the public API layer. It is actually always returned. Can we accept making it non-optional (breaking API change because it was optional before)? If not, DeviceResponse (this PR currently renames it directly to Device) can't be directly exposed and Device type needs to wrap it.
/// An object representing a device which can receive push notifications.
public struct Device: Codable, Equatable, Identifiable, Sendable {
private enum CodingKeys: String, CodingKey {
case id
case createdAt = "created_at"
}
/// The device identifier.
public let id: DeviceId
/// The date when the device for created.
public let createdAt: Date?
There was a problem hiding this comment.
I think I add a script to make it optional and v6 can make this small breaking change.
There was a problem hiding this comment.
Added post-processing step to make it back to optional in v5. In v6 we can properly fix it.
| "$file" | ||
| } | ||
| publicize_model AppSettings | ||
| publicize_model Device |
There was a problem hiding this comment.
OpenAPI models is not used as public model for Device type
| # Relax selected generated stored properties back to optional. Some models are | ||
| # exposed as public API where a property was historically optional (e.g. | ||
| # Device.createdAt was Date? before the OpenAPI migration). | ||
| optionalize_property() { | ||
| local file="$OUTPUT_DIR_CHAT/models/$1.swift" | ||
| sed -i '' -E \ | ||
| -e "s/^( let $2: [^?]+)$/\1?/" \ | ||
| "$file" | ||
| } | ||
| optionalize_property DeviceResponse createdAt |
There was a problem hiding this comment.
In v6 this can be removed. Because Device is now public model, but our current model has createdAt as optional value, I need to change the generated model to match the current public model for avoiding API breaking changes in v5. Of course, in v6 we delete this and make a small breaking change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Scripts/openapi_generate.sh (1)
159-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a verification check after the
sedsubstitution.If the regex doesn't match (e.g., the generator output format changes or the property is already optional),
sedsilently does nothing andcreatedAtstays non-optional. While the downstreamdevice.createdAt?.bridgeDateusage would catch this at compile time, a quick post-check would surface the problem immediately during codegen rather than later in the build.♻️ Suggested verification guard
optionalize_property() { local file="$OUTPUT_DIR_CHAT/models/$1.swift" + local before + before="$(grep -c " let $2: " "$file" || true)" sed -i '' -E \ -e "s/^( let $2: [^?]+)$/\1?/" \ "$file" + local after + after="$(grep -c " let $2: " "$file" || true)" + if [ "$before" -eq "$after" ]; then + echo "warning: optionalize_property: no substitution made for $2 in $1.swift" >&2 + fi }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Scripts/openapi_generate.sh` around lines 159 - 169, The optionalize_property helper in openapi_generate.sh should verify that the sed replacement actually changed the target property. After running the substitution for DeviceResponse.createdAt, add a post-check in optionalize_property that confirms the generated Swift file now contains an optional declaration for the requested symbol; if not, fail codegen immediately with a clear message. This keeps the fix localized to optionalize_property and guards against generator format changes or already-optional properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Scripts/openapi_generate.sh`:
- Around line 159-169: The optionalize_property helper in openapi_generate.sh
should verify that the sed replacement actually changed the target property.
After running the substitution for DeviceResponse.createdAt, add a post-check in
optionalize_property that confirms the generated Swift file now contains an
optional declaration for the requested symbol; if not, fail codegen immediately
with a clear message. This keeps the fix localized to optionalize_property and
guards against generator format changes or already-optional properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8b5b52a-2a9f-4512-8a59-a738ae87d61b
⛔ Files ignored due to path filters (1)
Sources/StreamChat/Generated/OpenAPI/models/Device.swiftis excluded by!**/generated/**
📒 Files selected for processing (2)
Scripts/openapi_generate.shSources/StreamChat/Database/DTOs/CurrentUserDTO.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/StreamChat/Database/DTOs/CurrentUserDTO.swift
Public Interface+ public final class Device: Sendable, Codable, JSONEncodable
+
+ case createdAt = "created_at"
+ case disabled
+ case disabledReason = "disabled_reason"
+ case hardwareId = "hardware_id"
+ case id
+ case pushProvider = "push_provider"
+ case pushProviderName = "push_provider_name"
+ case userId = "user_id"
+ case voip
+
+
+ public let createdAt: Date?
+ public let disabled: Bool?
+ public let disabledReason: String?
+ public let hardwareId: String?
+ public let id: String
+ public let pushProvider: String
+ public let pushProviderName: String?
+ public let userId: String
+ public let voip: Bool?
+ extension Device: Hashable
+
+ public static func ==(lhs: Device,rhs: Device)-> Bool
+ public func hash(into hasher: inout Hasher)
- public struct Device: Codable, Equatable, Identifiable, Sendable
-
- case id
- case createdAt = "created_at"
-
-
- public let id: DeviceId
- public let createdAt: Date?
|
|
Manually verified it (sample script and running demo app and inspecting device queries). Will merge it. |
SDK Size
|
StreamChat XCSize
Show 13 more objects
|
|
3 participants
🔗 Issue Links
Resolves: IOS-1871
🎯 Goal
Expose the full set of device data the backend returns through the public API instead of only
idandcreatedAt.📝 Summary
DeviceResponsemodel replaces the hand-writtenDevicetype as the public device model in the public API layer (renamed toDevice).Devicenow exposes additional data in the public interface:disabled,disabledReason,hardwareId,pushProvider,pushProviderName,userId, andvoip— in addition to the existingidandcreatedAt.CurrentChatUser.devices/currentDeviceandConnectedUser.loadDevices().🛠 Implementation
Scripts/openapi_generate.shrenames the generatedDeviceResponsetoDeviceand makes it public, following the same mechanism used forAppSettings.Device.createdAtis now a non-optionalDate(the backend always returns it); the remaining new fields follow the backend's optionality.DeviceDTOand the Core Data model gained the new attributes (additive, optional/defaulted → lightweight migration).Devicenow flows through both the connect payload and thelistDevicesendpoint, so the legacy internalDevicePayload/DeviceListPayloadwere removed.🎨 Showcase
N/A — no UI changes.
🧪 Manual Testing Notes
Launch the demo app and login (device gets added). No JSON parsing issues in the console.
Test function, can be copy-pasted into demo app and called at DemoAppCoordinator+DemoApp.swift line 81
☑️ Contributor Checklist
docs-contentrepoSummary by CodeRabbit
New Features
Bug Fixes