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.

Sign up for GitHub

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

Jump to bottom

fix: app crash on identify method #458

Merged
merged 3 commits into from
Jan 3, 2024
Merged

fix: app crash on identify method #458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb36c86
handling nil parameter
ami-aman Jan 2, 2024
1cba668
comment
ami-aman Jan 2, 2024
37eb49e
removed workaround
ami-aman Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 1 addition & 5 deletions Apps/APN-UIKit/APN UIKit/View/LoginViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,7 @@ class LoginViewController: BaseViewController {
}
storage.userEmailId = emailId
loginRouter?.routeToDashboard()
guard let data = body else {
CustomerIO.shared.identify(identifier: emailId)
return
}
CustomerIO.shared.identify(identifier: emailId, body: data)
CustomerIO.shared.identify(identifier: emailId, body: body)
}

@objc func settingsTapped() {
Expand Down
16 changes: 16 additions & 0 deletions Sources/Common/CustomerIOInstance.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,22 @@
identifier: String,
body: RequestBody
) {
// This code handles a specific scenario where a user
// unexpectedly provides a `nil` value for the RequestBody
// parameter, which the method isn't designed to handle directly.
// To prevent errors and ensure compatibility, this condition checks
// if the `body` is `nil`. If it is, it substitutes it with an
// EmptyRequestBody() object before passing it along to the
// identify(). This replacement safeguards against possible
// issues that could arise(eg. app crash). While the `identify`
// method itself provide polymorphic capabilities to handle `nil`,
// this specific check within this method offers an additional
// layer of protection and clarity for this case.
// (refer https://github.com/customerio/customerio-ios/blob/94cbf686c3c2a405534cfe908f7166558a3e0b5d/Sources/Tracking/CustomerIO.swift#L71-L75)
if let optionalValue = body as? Any?, optionalValue == nil {

Check warning on line 247 in Sources/Common/CustomerIOInstance.swift

View workflow job for this annotation

GitHub Actions / automated-tests 

conditional cast from 'RequestBody' to 'Any?' always succeeds
implementation?.identify(identifier: identifier, body: EmptyRequestBody())
return
Copy link
Contributor

Choose a reason for hiding this comment

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

personal preference opinion only,

if and else, over early return.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personal preference again but I would prefer guard which I think is more common in Swift and our codebase 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Response

Copy link
Contributor

Choose a reason for hiding this comment

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

i meant this, think this would have worked.

if let optionalValue = body as? Any?, optionalValue == nil {
            implementation?.identify(identifier: identifier, body: EmptyRequestBody())
        } else {
            implementation?.identify(identifier: identifier, body: body)
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this would have worked. I understand it is a personal preference but does it affect the business logic and working of the fix ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No it doesn't and hence, didn't say so either. 😄

I was just responding to this reply, that the personal preference I was mentioning was utilizing the same logic for comparison. We choose to go with it or not thats another thing.

I do agree that there could be personal preferences but swift doesn't allow the conversion and hence the alternative approach has been followed here for comparison of a non-nil property with nil.

}
implementation?.identify(identifier: identifier, body: body)
}

Expand Down Expand Up @@ -300,7 +316,7 @@
name: String,
data: [String: Any]
) {
guard let logger = diGraph?.logger else {

Check warning on line 319 in Sources/Common/CustomerIOInstance.swift

View workflow job for this annotation

GitHub Actions / automated-tests 

value 'logger' was defined but never used; consider replacing with boolean test
return
}
implementation?.screen(name: name, data: data)
Expand Down