fix(cloud_firestore_odm): add support for searching Enums

[mrorabau]

Description

This PR should add support for searching Enums [.whereMyEnum()]. Previously using an Enum in a model would be stored and retrieved, but then it couldn't be searched on.

Related Issues

Closes https://github.com/firebase/flutterfire/issues/8338

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• [x] All existing and new tests are passing.
• [ ] I updated/added relevant documentation (doc comments with ///).
• [x] The analyzer (melos run analyze) does not report any problems on my PR.
• [x] I read and followed the Flutter Style Guide.
• [x] I signed the CLA.
• [x] I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• [ ] Yes, this is a breaking change.
• [x] No, this is not a breaking change.

[google-cla[bot]]

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).

For more information, open the CLA check for this pull request.

[rrousselGit]

Hello!

Could you add tests for this? Thanks 😄

[mrorabau]

@rrousselGit I will try and add a test next week if you would like. I wasn't sure if you wanted to review it first. (I'm out of town at the moment, sorry I can't do it sooner).

[google-oss-bot]

Hey @mrorabau. We need more information to resolve this issue but there hasn't been an update in 7 weekdays. I'm marking the issue as stale and if there are no new updates in the next 7 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

[mrorabau]

@google-oss-bot I'm working on the tests (as requested). Having some issues.

[mrorabau]

@rrousselGit If you could take a look at this and see what else is needed. I tried to use the same type of tests you had, however it seems that it only tests for expected compile errors.

[mrorabau]

@Salakar Thanks. I'll look into the error (unless you see it quickly). It is strange that I didn't get that on my end before submitting. There was a merge conflict, but I thought I resolved it.

[russellwheatley]

Hey @mrorabau, I'm not sure what is happening but you've reformatted the entirety of the iOS code base 🤔. Do you mind updating to the latest flutter stable (3.0.0), clang-format & swiftformat and running melos run format, please?

[mrorabau]

Do you mind updating to the latest flutter stable (3.0.0), clang-format & swiftformat and running melos run format, please?

@rrousselGit Yeah, I will run it again (I'm on 3.0.0 now). I noticed that it reformatted (almost) everything, but it was the melos run format that did it (I followed the instructions to submit it). Any chance that you have a wider column set somewhere that isn't in the repo? I ask because it seemed that almost all of the format changes were wrapping related.

[mrorabau]

@rrousselGit Sorry if I'm bugging you, but a couple of things:

• The automated apple tests appear to be failing because of an emulator problem
• The test "test/document_reference_test.dart" works for me, but fails in the automated test

• The Apple/MacOS files still get formatted wrong. Maybe it has something to do with the line-wrap? It seems that the code in master is not wrapped as early as it does for me.
  • flutter_plugin_tools 0.8.6
  • swiftformat 0.49.9
  • clang-format 14.0.4-++20220520113355+5f66e721ec1d-1~exp1~20220520113441.138

[mrorabau]

@rrousselGit I did a clean checkout and moved only the files I modified and did not run the formatter so the IOS files are no longer modified by whatever in the swift formatter caused it to go crazy.

[rrousselGit]

Hello! I'll review it next week. I'd like to merge the other pending ODM PRs first

[rrousselGit]

I don't have the permission to push to your PR. Could you fix the conflicts?

[mrorabau]

Hi Remi,

I've de-conflicted it. So please take a look. BTW, your updates I think will help a lot. I had started a fix for timestamps/datetime but it looks like you already got to that.

-mark

On Tue, Jul 12, 2022 at 3:38 AM Remi Rousselet @.***> wrote:

I don't have the permission to push to your PR. Could you fix the conflicts?

— Reply to this email directly, view it on GitHub https://github.com/firebase/flutterfire/pull/8386#issuecomment-1181423382, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANWRA5HLNKE3DRJM7LUWITVTUOGTANCNFSM5SMIEBKA . You are receiving this because you were mentioned.Message ID: @.***>

[rrousselGit]

LGTM after the last test is added. Thanks for your work!

[mrorabau]

@rrousselGit Sorry this took a bit. I think all of the tests should be in now. The latest supports:

• Field of type Enum
• Field of type List< Enum >
• Field of type List<Map<Enum, String>>

It is not currently recursive (does not go deeper than a list of maps of enums). I think this is a good start. Please let me know. I also changed element -> element2 for the stuff I added as well.

We'd really like to get this in, as we are using it in our own code.

Thanks!

[mrorabau]

@russellwheatley @rrousselGit I know you guys are busy all the time, but is there something else you think I need to do in order to get this merged?

Thanks!

[rrousselGit]

Hello! Sorry for the delay. We've been pretty busy with the Flutter Vikings. Let's do what we ca to merge this.

[rrousselGit]

I don't like the changes introduced for List<Enum> & co.

cloud_firestore_odm currently voluntarily doesn't contain logic for how to encode an object. Now that Dart has enum "classes", I'd instead request users to do:

enum Example {
  a(),
  b();

  const Example();

  factory Example.fromJson(Object? obj) {}

  Object? toJson() {}
}

If necessary, we can make a PR to json_serializable to add whatever is missing for this.

[rrousselGit]

Could you maybe remove the logic for List<Enum> & co? These are a bit controversial. We could merge simple enum support without those first

Also I think it'd be valuable to mention in the docs that json_serializable's options aren't considered yet for enums.

Could you also please allow FlutterFire maintainers to push to your branch? Otherwise I can't make changes to this PR.

[mrorabau]

I don't like the changes introduced for List<Enum> & co.

cloud_firestore_odm currently voluntarily doesn't contain logic for how to encode an object.

I admit that the Lists and Maps of Enums are a bit hacky. I actually started this before the Enum object was a thing. The biggest issue originally was that ODM would not create any .where / .orderBy logic for anything that had Enums (or back then DateTime for that matter). Using json_serializable didn't cure that issue.

Now that Dart has enum "classes", I'd instead request users to do:

enum Example {
  a(),
  b();

  const Example();

  factory Example.fromJson(Object? obj) {}

  Object? toJson() {}
}

If necessary, we can make a PR to json_serializable to add whatever is missing for this.

Ok, I can look at that. I also agree it will ultimately be best to support full Enum objects if we can get that to work. I suppose part of this was limiting how far down the rabbit-hole I wanted to go. It has already been hard enough to get anything pushed through on the ODM, adding additional libraries (json_serializable, or even the firebase core) seemed like it might be even more nightmarish.

[mrorabau]

Could you maybe remove the logic for List<Enum> & co? These are a bit controversial. We could merge simple enum support without those first

This is certainly possible (and was the first submission). The issue is that for our own project (greedy reason, I know) we are HEAVILY using List<Enum> and even List<Map<Enum, String>>. I will need a solution to that for our code base -- somehow.

One of the more recent changes you added back in July was adding Coverters (FirestoreDateTimeConverter, etc.). Since JsonEnum is now supported, should a FirestoreEnumConverter be added? I think the big issue is still making sure that the .where / .orderBy clauses work.

Also I think it'd be valuable to mention in the docs that json_serializable's options aren't considered yet for enums.

I didn't write any docs yet, because I honestly wanted some of you/maintainers' input first. The json_serializable seems to claim that it will support Enums recursively, so if we can ensure that this is true, and that we can not reject Enums (or List<Enum>, List<Map<Enum, E>>, etc.) then we should in theory be ok. I could always seem to get it to store Enums, just not retrieve them.

Could you also please allow FlutterFire maintainers to push to your branch? Otherwise I can't make changes to this PR.

Sorry about that, I thought I had added you (@rrousselGit) to the repo already, but I guess I didn't. I also couldn't figure out how to add any sort of flutterfire maintainers team. If you have a suggestion for that, please let me know.

[rrousselGit]

It has already been hard enough to get anything pushed through on the ODM

I apologize for the slow review. These past few months have been very busy

Sorry about that, I thought I had added you (@rrousselGit) to the repo already, but I guess I didn't. I also couldn't figure out how to add any sort of flutterfire maintainers team. If you have a suggestion for that, please let me know.

Thanks! In theory you didn't have to invite me to the repo. Github offers a checkbox to allow maintainers to contribute to the branch, which you might have checked off.

---

About List/Map support:

I think we can have a special case for List<Enum>. That should be fine.

My real issue is with List<Map<Enum, ...>>.
List<Enum> makes sense as we can do things like whereList(arrayContains: Enum.value). But I'm not sure what the Map version is about. As long as you're using json_serializable, its @JsonValue should allow the de/serialization of enums.

[rrousselGit]

Ultimately I think this PR tries to do two different things:

• It adds support for whereEnum/orderByEnum & list variant
• It tries to add support for complex objects in various methods, such as update(property: Complex())

The changes for the first feature are fairly uncontroversial. I'd be open to merging the PR if it did just that

The second part though is a different matter. This PR doesn't really solve the problem, but rather covers only the scenarios you personally need.
As such I don't think I can accept those changes.

I think a simple way forward would be to remove the changes for 2, and in your project you can keep using your fork for now.

The true solution for 2 would be to raise a PR to json_serializable similar to https://github.com/google/json_serializable.dart/pull/1164, but for generating a "toJson" method for properties. This would likely require discussing a bit with Kevin.
I have some leeway now that FlutterVikings is done. So I can look into making the PR to json_serializable and fixing the issue.

Does that sound reasonable?

[rrousselGit]

I've created an issue on json_serializable related to what would be needed for truly fixing 2: https://github.com/google/json_serializable.dart/issues/1198

[mrorabau]

Ultimately I think this PR tries to do two different things:

• It adds support for whereEnum/orderByEnum & list variant
• It tries to add support for complex objects in various methods, such as update(property: Complex())

Yes, I think that is about right. #1 is the most important, as we can have some (expensive) work arounds for #2.

The changes for the first feature are fairly uncontroversial. I'd be open to merging the PR if it did just that

The second part though is a different matter. This PR doesn't really solve the problem, but rather covers only the scenarios you personally need. As such I don't think I can accept those changes.

Yes, I made a caveat about that, and said I could make handle more, but I wasn't sure if it was the right path.

I think a simple way forward would be to remove the changes for 2, and in your project you can keep using your fork for now.

We can probably do that. Worse case we can always make our own (expensive) update by getting the doc, merging it in code (on our end), deleting the old entry and re-adding it. However it isn't pretty, expensive, and really probably shouldn't be done without a transaction.

The true solution for 2 would be to raise a PR to json_serializable similar to google/json_serializable.dart#1164, but for generating a "toJson" method for properties. This would likely require discussing a bit with Kevin. I have some leeway now that FlutterVikings is done. So I can look into making the PR to json_serializable and fixing the issue.

I agree and think this sounds like the best solution.

Does that sound reasonable?

Absolutely. Thanks for you continued assistance!

[mrorabau]

It has already been hard enough to get anything pushed through on the ODM

I apologize for the slow review. These past few months have been very busy

Please don't take it personally, it wasn't meant as a complaint. I was just saying I was worried about going to deep with this stuff without some maintainer buy-in.

About List/Map support:

I think we can have a special case for List<Enum>. That should be fine.

My real issue is with List<Map<Enum, ...>>. List<Enum> makes sense as we can do things like whereList(arrayContains: Enum.value). But I'm not sure what the Map version is about. As long as you're using json_serializable, its @JsonValue should allow the de/serialization of enums.

I have to try and remember now, but I think that the issue was again with .where and .orderBy (along with update). We were also using Map<Enum, String> and List<Map<Enum, String>>. In all cases, it seems that json_serializable would handle the storage/retrieval of Enums (which was great), however we had problems with things similar to the following:

• .whereFieldMap(isEqual: Map()) // Rare but sometimes needed for an exact match
• .whereFieldListMap(contains: Map(Enum, String)) // This is used a lot for us.

A better example of the later would be something like this:

[
   {MyEnum.Text: "Some text"},
   {MyEnum.Image: "Some URL"},
   ...
]

.whereFieldListMap(contains: {MyEnum.Text: "Some text"})

I always thought there needs to be a more generic way of handling this while still performing the type checking. I wasn't sure if we could somehow use the FieldMap for this?

[mrorabau]

I wasn't sure if we could somehow use the FieldMap for this?

Commenting to myself, but I think I now see that you @rrousselGit are doing exactly that with your xFieldMap proposal. Sorry I missed that earlier.

[rrousselGit]

We can probably do that. Worse case we can always make our own (expensive) update by getting the doc, merging it in code (on our end), deleting the old entry and re-adding it. However it isn't pretty, expensive, and really probably shouldn't be done without a transaction.

No need for that. The ODM exposes the raw DocumentReference so you can call the native "update":

odmCollection.doc('42').reference.update({"whatever": Complex().toJson()});

It kind of eject the ODM for this operation, but you can at least move forward

[mrorabau]

odmCollection.doc('42').reference.update({"whatever": Complex().toJson()});

It kind of eject the ODM for this operation, but you can at least move forward

I'm feeling pretty stupid for forgetting that :)