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

Support hex strings to prevent broken log lines #13128

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

ls-todd-lunter
Copy link
Contributor

@ls-todd-lunter ls-todd-lunter commented Jun 13, 2024

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.

Copy link

google-cla bot commented Jun 13, 2024

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.

@paulb777
Copy link
Member

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

@paulb777
Copy link
Member

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

@paulb777 paulb777 closed this Jul 12, 2024
@ls-todd-lunter
Copy link
Contributor Author

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

@paulb777
Copy link
Member

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.

@ls-todd-lunter ls-todd-lunter force-pushed the fix-hex-logging branch 3 times, most recently from 53520d3 to a6bd5b6 Compare July 15, 2024 16:15
@ls-todd-lunter
Copy link
Contributor Author

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

@paulb777
Copy link
Member

@wu-hui PTAL

@ls-todd-lunter
Copy link
Contributor Author

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.

Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@paulb777 paulb777 added this to the 11.2.0 - M153 milestone Aug 26, 2024
@paulb777 paulb777 merged commit cc4f0cb into firebase:main Aug 26, 2024
29 checks passed
mergify bot referenced this pull request in cgrindel/rules_swift_package_manager Sep 16, 2024
….2.0" (#1228)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[firebase/firebase-ios-sdk](https://redirect.github.com/firebase/firebase-ios-sdk)
| minor | `from: "11.1.0"` -> `from: "11.2.0"` |

---

### Release Notes

<details>
<summary>firebase/firebase-ios-sdk (firebase/firebase-ios-sdk)</summary>

###
[`v11.2.0`](https://redirect.github.com/firebase/firebase-ios-sdk/releases/tag/11.2.0):
Firebase Apple 11.2.0

[Compare
Source](https://redirect.github.com/firebase/firebase-ios-sdk/compare/11.1.0...11.2.0)

The Firebase Apple SDK (11.2.0) is now available. For more details, see
the [Firebase Apple SDK release
notes.](https://firebase.google.com/support/release-notes/ios#11.2.0)

To install this SDK, see [Add Firebase to your
project](https://firebase.google.com/docs/ios/setup).

#### What's Changed

- \[Auth] Phone Auth – Fallback to reCATCHA flow when "invalid app
credential" error is thrown by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13519](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13519)
- \[Auth] Fix Xcode 16 continuation crashes by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13521](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13521)
- \[Auth] Fix Phone Auth via APNS for Sandbox Tokens and update Sample's
Firebase app by [@&#8203;paulb777](https://redirect.github.com/paulb777)
in
[https://github.com/firebase/firebase-ios-sdk/pull/13539](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13539)
- \[Auth] Add background modes capability to plist by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13548](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13548)
- \[Auth] When swizzling is disabled, open URLs via SceneDelegate by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13557](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13557)
- \[Auth] Fix unexpected nil in fetchSignInMethods success case by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13561](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13561)
- \[Auth] Fix user session persistence in multi tenant projects by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13567](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13567)
- \[Crashlytics] Fix Firebase/Crashlytics min iOS version by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13580](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13580)
- \[Database] Fix temporary disconnect when app goes inactive by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13564](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13564)
- \[Firestore] Mark readonly public classes as Sendable by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13453](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13453)
- \[Firestore] Support hex strings to prevent broken log lines by
[@&#8203;ls-todd-lunter](https://redirect.github.com/ls-todd-lunter) in
[https://github.com/firebase/firebase-ios-sdk/pull/13128](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13128)
- \[Functions] `FunctionsContext` Updates by
[@&#8203;yakovmanshin](https://redirect.github.com/yakovmanshin) in
[https://github.com/firebase/firebase-ios-sdk/pull/13531](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13531)
- \[Functions] Updated Functions Errors by
[@&#8203;yakovmanshin](https://redirect.github.com/yakovmanshin) in
[https://github.com/firebase/firebase-ios-sdk/pull/13535](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13535)
- \[Testing] Update OCMock dependency to v3.9.4 by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13536](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13536)
- \[Vertex AI] Make `uri` optional in `Citation` and add `title` field
by [@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13520](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13520)
- \[Vertex AI] Add `Sendable` conformance to types by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13560](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13560)
- \[Vertex AI] Make `Logger` properties constants by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13570](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13570)
- \[Vertex AI] Make `GenerativeModel` and `Chat` into Swift actors by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13545](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13545)
- \[Vertex AI] Make generateContentStream/sendMessageStream throws by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13573](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13573)
- \[Vertex AI] Add `SourceImage` enum to `ImageConversionError` by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13575](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13575)
- \[Vertex AI] Add `responseSchema` to `GenerationConfig` by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13576](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13576)

#### New Contributors

- [@&#8203;ls-todd-lunter](https://redirect.github.com/ls-todd-lunter)
made their first contribution in
[https://github.com/firebase/firebase-ios-sdk/pull/13128](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13128)

**Full Changelog**:
firebase/firebase-ios-sdk@11.1.0...11.2.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config
help](https://redirect.github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://redirect.github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: cgrindel-self-hosted-renovate[bot] <139595543+cgrindel-self-hosted-renovate[bot]@users.noreply.github.com>
Co-authored-by: Chuck Grindel <chuck.grindel@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@firebase firebase locked and limited conversation to collaborators Sep 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants