firebase-ios-sdk icon indicating copy to clipboard operation
firebase-ios-sdk copied to clipboard

Support hex strings to prevent broken log lines

Open ls-todd-lunter opened this issue 1 year ago • 2 comments

Discussion

While trying to record Firestore logs within our app, we started to notice many log lines were being reported as (null) in the Console. This seems to be because when logging the reference to the C++ objects, the %s format was adding non-UTF8 encodable characters into the string. This meant when using MakeNSString in the Firestore logger, those non-UTF8 characters failed the string initialization, and it returns nil.

Since there is already quite a lot of use of absl in the repo, I figured we could quick-win, use that to convert the string to hex and record that instead. I suspect logging the object was meant just to see if the value drifted overtime, rather than anything intrinsic to the object itself, so the hex value should do the same thing.

Testing

I only added a test for the record hex value itself. Otherwise, it will behave the same for other types.

API Changes

None, internal to the SDK.

ls-todd-lunter avatar Jun 13 '24 22:06 ls-todd-lunter

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jun 13 '24 22:06 google-cla[bot]

Thanks for the PR! Please sign the CLA and we can take a look.

paulb777 avatar Jun 15 '24 17:06 paulb777

Closing due to missing CLA. We can reopen when ready.

paulb777 avatar Jul 12 '24 23:07 paulb777

Hi @paulb777 apologies, we finally got the CLA resolved! That check is now green.

ls-todd-lunter avatar Jul 14 '24 10:07 ls-todd-lunter

Thanks @ls-todd-lunter! - Please address the code style issues showing up in the check CI - Most likely from four-space vs two-space indenting.

paulb777 avatar Jul 14 '24 16:07 paulb777

Rebased on main to see if it resolves the test failure. Otherwise, I don't think my logging changes would've affected the FIRCompositeIndexQueryTests.

ls-todd-lunter avatar Jul 15 '24 16:07 ls-todd-lunter

@wu-hui PTAL

paulb777 avatar Jul 15 '24 20:07 paulb777

Rebased on latest release. @wu-hui if you could take a look, this still prevents us from using the current release since some logs fail to be converted to NSStrings.

ls-todd-lunter avatar Aug 26 '24 18:08 ls-todd-lunter