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

Public Auto Persistent Index Creation API #11757

Merged
merged 11 commits into from
Sep 5, 2023
Merged

Public Auto Persistent Index Creation API #11757

merged 11 commits into from
Sep 5, 2023

Conversation

cherylEnkidu
Copy link
Contributor

No description provided.

@google-oss-bot
Copy link

google-oss-bot commented Aug 30, 2023 •
edited
Loading

1 Warning
⚠️ New public headers were added, did you remember to add them to the umbrella header?

Generated by 🚫 Danger

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023 •
edited
Loading

Apple API Diff Report

Commit: d7dd4ac
Last updated: Mon Sep 18 22:20 PDT 2023
View workflow logs & download artifacts

FirebaseFirestore

Classes

FIRFirestore
[ADDED] persistentCacheIndexManager 
Swift:
+  var persistentCacheIndexManager : FIRPersistentCacheIndexManager ? { get }
Objective-C:
+  @property ( nonatomic , readonly , nullable ) FIRPersistentCacheIndexManager * persistentCacheIndexManager ;

@google-oss-bot
Copy link

google-oss-bot commented Aug 30, 2023 •
edited
Loading

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestore.framework

    Overall coverage changed from 88.11% (acfd28c) to 88.13% (d7dd4ac) by +0.02%.

    FilenameBase (acfd28c)Merge (d7dd4ac)Diff
    filesystem_posix.cc79.09%81.82%+2.73%
    FIRFirestore.mm88.97%89.02%+0.05%
    ordered_code.cc93.90%94.39%+0.49%
    task.cc93.91%94.78%+0.87%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/S0J7wW4god.html

@cherylEnkidu cherylEnkidu changed the title Public csi Public Auto Persistent Index Creation API Aug 31, 2023
markarndt
markarndt approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@markarndt markarndt left a comment

Choose a reason for hiding this comment

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

A few minor edits.

Firestore/CHANGELOG.md Outdated Show resolved Hide resolved
Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h Outdated Show resolved Hide resolved
Firestore/Source/Public/FirebaseFirestore/FIRPersistentCacheIndexManager.h Outdated Show resolved Hide resolved
Firestore/Source/Public/FirebaseFirestore/FIRPersistentCacheIndexManager.h Outdated Show resolved Hide resolved
Firestore/Source/Public/FirebaseFirestore/FIRPersistentCacheIndexManager.h Outdated Show resolved Hide resolved
@cherylEnkidu
Copy link
Contributor Author

@wu-hui In DatabaseTests.swift, if I call FirebaseApp.configure() in two tests, it will lead to error:app configure twice. However, the equivalent objective c tests didn't have this problem. I think there are some issues with testing framework but it is not easy to debug. So I combine two tests from objective c into a single one in swift, which guarantee the test coverage.

Given this problem is not related to csi, my judgement is it should not block this pr to merge. But I will look into it in the meantime to see if I can find a better solution.

@cherylEnkidu
Copy link
Contributor Author

The difference between Objective C and Swift is because Swift test didn't tear down APP for each test like this one:

firebase-ios-sdk/Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm

Line 50 in acfd28c

[FIRApp resetApps];

Currently Swift can only test against public api and the resetApp is internal use only.

@cherylEnkidu cherylEnkidu merged commit 065a5bb into master Sep 5, 2023
41 checks passed
@cherylEnkidu cherylEnkidu deleted the cheryllin/publicCSI branch September 5, 2023 21:49
ncooke3
ncooke3 reviewed Sep 13, 2023
View reviewed changes
Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h
Comment on lines +134 to +137
NS_SWIFT_NAME(setIndexConfiguration(_:completion:)) DEPRECATED_MSG_ATTRIBUTE(
"Instead of creating cache indexes manually, consider using "
"`PersistentCacheIndexManager.enableIndexAutoCreation()` to let the SDK decide whether to "
"create cache indexes for queries running locally.");
Copy link
Member

Choose a reason for hiding this comment

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

@sunmou99, it'd be a nice-to-have if API diff tool caught deprecations as well. Here, the ObjC and Swift API were both deprecated.

andrewheard pushed a commit that referenced this pull request Sep 20, 2023
@cherylEnkidu @ncooke3 @andrewheard
Public Auto Persistent Index Creation API (#11757)
3df8b66 
Co-authored-by: Nick Cooke <[email protected]>
@firebase firebase locked and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ncooke3 ncooke3 ncooke3 left review comments

@wu-hui wu-hui wu-hui approved these changes

@markarndt markarndt markarndt approved these changes

Assignees

@markarndt markarndt

@wu-hui wu-hui

Labels
api: firestore public-api-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cherylEnkidu @google-oss-bot @ncooke3 @markarndt @wu-hui