Skip to content

feat: Add support for new Firestore types #14800

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

ehsannas
Copy link
Contributor

@ehsannas ehsannas commented May 1, 2025

Adds support for MinKey, MaxKey, RegexValue, Int32Value, BsonObjectId, BsonTimestamp, and BsonBinaryData.

ehsannas added 3 commits May 1, 2025 14:20
* Add RegexValue class.

* Add to/from proto logic.

* Add comparison logic and unit tests.

* Add serializer unit test.

* Add Codable support, Improve Swift API, Add integration test.

* Add more unit tests and integration tests.

Also fixed a bug found by unit tests.

* Add 6 other types' public API.

* rename `.m` to `.mm` to stay consistent.

* More unit tests.

* WIP: Add the FieldValue methods for building new types.

* Add the Swift API and the Obj-C Unit tests.

* Add UserDataReader support for new types.

* Add missing `extern`.

* Add UserDataWriter support for types. Int32 is still missing.

* UserDataWriter support for int32.

* Update TypeOrder usages with new types.

* Add comparison logic for more types.

* Add Int32Value comparison logic.

* Add SerializerTests for more types.

* Use snake case.

* Add more unit tests.

* Fix bug and add integration test.

* Add more integration tests.

* Add more integration tests.

* Add more integration and unit tests.

* Expose public `isEqual` for new types and add tests for it.

* Add cross-type order test.

* Add Codable support along with integration tests for them.

* Remove named parameter for Int32Value and BsonObjectId.

* clang-format.

* Use `uint8_t` for BsonBinaryData's subtype.

* Add `description` for new types.

* Reuse type check logic for new types.

* Use uint8_t for BsonBinaryData.

* Adds tests for FIRFieldValue static c'tors of new types.

* Add a few missing tests.

* Update tests to check listeners, src=cache, src=server.

* Remove FieldValue factory methods.
* Add RegexValue class.

* Add to/from proto logic.

* Add comparison logic and unit tests.

* Add serializer unit test.

* Add Codable support, Improve Swift API, Add integration test.

* Add more unit tests and integration tests.

Also fixed a bug found by unit tests.

* Add 6 other types' public API.

* rename `.m` to `.mm` to stay consistent.

* More unit tests.

* WIP: Add the FieldValue methods for building new types.

* Add the Swift API and the Obj-C Unit tests.

* Add UserDataReader support for new types.

* Add missing `extern`.

* Add UserDataWriter support for types. Int32 is still missing.

* UserDataWriter support for int32.

* Update TypeOrder usages with new types.

* Add comparison logic for more types.

* Add Int32Value comparison logic.

* Add SerializerTests for more types.

* Use snake case.

* Add more unit tests.

* Fix bug and add integration test.

* Add more integration tests.

* Add more integration tests.

* Add more integration and unit tests.

* Expose public `isEqual` for new types and add tests for it.

* Add cross-type order test.

* Add Codable support along with integration tests for them.

* Remove named parameter for Int32Value and BsonObjectId.

* clang-format.

* Use `uint8_t` for BsonBinaryData's subtype.

* Add `description` for new types.

* Reuse type check logic for new types.

* Use uint8_t for BsonBinaryData.

* Adds tests for FIRFieldValue static c'tors of new types.

* Add a few missing tests.

* Update tests to check listeners, src=cache, src=server.

* Implement indexing for bson types.

* Add test for offline write and read from cache.

* WIP: Adding more unit tests and integration tests.

* Add IndexAllTypesTogether test.

* Properly handle empty segments in ref.

* Add ComputesLowerBound test.

* Add ComputesUpperBound test.

* Add IndexValueWriterTest.

* Add bson types to index_value_writer_test.cc.

* Add CSI tests for bson types.

* Check ordered results for queries served from the index.

* Add TODO for failing tests.

* Address comments.

* Improve doc ordering in the new leveldb_local_store_test tests.
Copy link
Contributor

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

@google-oss-bot
Copy link

google-oss-bot commented May 1, 2025

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

Generated by 🚫 Danger

@ehsannas ehsannas self-assigned this May 1, 2025
@ehsannas ehsannas requested a review from ncooke3 May 9, 2025 23:43
@ehsannas
Copy link
Contributor Author

TODO (ehsann): Update Bson* -> BSON*

Comment on lines +2 to +3
- [feature] Adds support for the following new types: MinKey, MaxKey, RegexValue, Int32Value, BsonObjectId,
BsonTimestamp, and BsonBinaryData. (#14800)
Copy link
Member

Choose a reason for hiding this comment

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

nit: code backticks and updated Bson to BSON

Suggested change
- [feature] Adds support for the following new types: MinKey, MaxKey, RegexValue, Int32Value, BsonObjectId,
BsonTimestamp, and BsonBinaryData. (#14800)
- [feature] Adds support for the following new types: `MinKey`, `MaxKey`, `RegexValue`, `Int32Value`, `BSONObjectId`,
`BSONTimestamp`, and `BSONBinaryData`. (#14800)

Copy link
Member

Choose a reason for hiding this comment

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

FYI, these build failures are expected. I think it's acceptable to merge with the spm-binary workflow failing, and I'll fix right after. Another option is to build a zip off of this branch but that's only worth doing when the PR is final. Either way works–– we just need to update the SPM FST binary to include the new sources.


#include "Firestore/Source/Public/FirebaseFirestore/FIRBsonBinaryData.h"

NS_ASSUME_NONNULL_BEGIN
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove NS_ASSUME_NONNULL_BEGIN and NS_ASSUME_NONNULL_END from .m or .mm files. These macros only have an effect when used in header files.

self = [super init];
if (self) {
_subtype = subtype;
_data = data;
Copy link
Member

Choose a reason for hiding this comment

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

@property(atomic, copy, readonly) NSData *data;

When initializing a property with the copy attribute, the property setter that does the copy doesn't run when accessing the ivar with the _ prefix. So the copy needs to be manual here:

Suggested change
_data = data;
_data = [data copy];

Good article: https://furbo.org/2012/05/04/arc-and-copy/

Since this and the other types are immutable and don't re-set the properties after initialization, the copy isn't doing anything. I'm okay with removing it or leaving it as-is as documentation of the intended ownership strategy.

* @param subtype An 8-bit unsigned integer denoting the subtype of the data.
* @param data The binary data.
*/
- (instancetype)initWithSubtype:(uint8_t)subtype data:(nonnull NSData *)data;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think the warning has to do with this line. The class interface is wrapped with NS_ASSUME_NONNULL_BEGIN which auto assumes every param is nonnull already

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, the nonnull could be removed as NS_ASSUME_NONNULL_BEGIN is adding it for you.

The issue is that
- (BOOL)isEqual:(id)object; should actually be - (BOOL)isEqual:(nullable id)object;. Added suggestion below.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the nonnulls here are duplicates since the class assumes nonnull

- (instancetype)initWithSubtype:(uint8_t)subtype data:(nonnull NSData *)data;

/** Returns true if the given object is equal to this, and false otherwise. */
- (BOOL)isEqual:(id)object;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- (BOOL)isEqual:(id)object;
- (BOOL)isEqual:(nullable id)object;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants