🔥 [🐛] ignoreUndefinedProperties not respected for arrays of objects

[pehagg]

Issue

According to what I'm seeing, the ignoreUndefinedProperties setting is not respected for objects in arrays, i.e. if you have an object in an array that has one or more undefined values, then you'll get "Unsupported field value: undefined", even if you've set ignoreUndefinedProperties to true.

I tracked down the issue and it could be fixed by adding ignoreUndefined to buildNativeArray and passing it along to generateNativeData, as well as adding the argument to the buildNativeArray call in generateNativeData at line 108 (or thereabouts).

Here lies a hidden question, should the arrays be checked like this or not? I'm not familiar with how the JS SDK behaves and nor do I have the time or the interest too look into it right now.

---

Project Files

Javascript

Click To Expand

package.json:

# N/A

firebase.json for react-native-firebase v6:

# N/A

iOS

Click To Expand

ios/Podfile:

• I'm not using Pods
• I'm using Pods and my Podfile looks like:

# N/A

AppDelegate.m:

// N/A

---

Android

Click To Expand

Have you converted to AndroidX?

• my application is an AndroidX application?
• I am using android/gradle.settings jetifier=true for Android compatibility?
• I am using the NPM package jetifier for react-native compatibility?

android/build.gradle:

// N/A

android/app/build.gradle:

// N/A

android/settings.gradle:

// N/A

MainApplication.java:

// N/A

AndroidManifest.xml:

<!-- N/A -->

---

Environment

Click To Expand

react-native info output:

 OUTPUT GOES HERE

• Platform that you're experiencing the issue on:
  • iOS
  • Android
  • iOS but have not tested behavior on Android
  • Android but have not tested behavior on iOS
  • Both
• react-native-firebase version you're using that has this issue:
  • 12.1.0
• Firebase module(s) you're using that has the issue:
  • Firestore
• Are you using TypeScript?
  • N/A

---

• 👉 Check out React Native Firebase and Invertase on Twitter for updates on the library.

[mikehardy]

interesting! I can't imagine why we'd have a toggle to ignore them and still see that message ever, I'll be surprised if firebase-js-sdk works that way. Marked for review but I'm guessing you are correct and I appreciate the solution sketch

[pehagg]

[...] I can't imagine why we'd have a toggle to ignore them and still see that message ever, I'll be surprised if firebase-js-sdk works that way. [...]

That's my assumption as well, but believe me, I've seen some really weird SDKs during my 20 years as a developer.

[mikehardy]

[...] I can't imagine why we'd have a toggle to ignore them and still see that message ever, I'll be surprised if firebase-js-sdk works that way. [...]

That's my assumption as well, but believe me, I've seen some really weird SDKs during my 20 years as a developer.

Preach 🙏 😆

[RayVR]

this is requiring a decent amount of work to deal with. Is this planned for a BF release any time soon or should I downgrade to the last version that did not have this issue?

[mikehardy]

Hey @RayVR or @pehagg - I'm investigating this and want to add a test case to make sure I get it right. I'm pretty sure I understand it well but just to make sure could one of you provide a sample JSON object that unexpectedly fails when you have ignoreUndefinedProperties true and send it in?

[TMcLoone]

@mikehardy Here's an example of something that fails for me:

[{name: 'Tim', location: { state: undefined, country: 'United Kingdom'} }]

I haven't tested it to see if it fails when it's not nested but by the sounds of the others it does.

[mikehardy]

About as simple as I expected ;-) but happy to have a known-bad example, thank you @TMcLoone !

[pehagg]

Sorry for not replying earlier, haven't been at the keyboard since Thursday noon. Can confirm that @TMcLoone 's example is in-line with our code that triggers the issue.

[RayVR]

@mikehardy Glad to hear there's a new testcase for this. What is the bug fix release cycle like for this project? I'm unfortunately running into this issue frequently in unexpected places.

[mikehardy]

Sorry there's no published fix yet. As with all open source the cycle is either someone proposes a fix from the community and I can merge and release in the same day plus there's an auto generated set of patch package patches, or it waits on me or another maintainer with no ETA given. The feature was added recently as well, it may be trivial to simply go in and reverse it or use the older version temporarily

Without giving an ETA as those simply lead to disappointment in open source I can say it's personally a high priority and having a reproduction is typically the biggest hurdle. We already have that

[mikehardy]

Your patience has been greatly appreciated! I'm slowly catching up over here in firebase-land and I think I've got this one. Most of the PR I just posted is testing support (thanks again for the sample, obvious though it is, it was useful to be sure)

I'd appreciate if someone could check the PR and maybe integrate the patch-package set to verify? It's the first patch post-release so it's super clean -

[edit - re-pushed the PR, has new links if you were an early-bird and already looked here]

• https://github.com/invertase/react-native-firebase/suites/3264275010/artifacts/75970553
• from https://github.com/invertase/react-native-firebase/actions/runs/1040823227

[mikehardy]

This seemed important and is a real problem but I haven't heard back from anyone? In the absence of confirmation I will assume my fix is good and will merge and release it later today

[RayVR]

Mike, Thank you. This is important as I have a half-baked solution to avoid crashes in production but a real solution will restore functionality. Thank you!

…

On Jul 21, 2021, at 23:30, Mike Hardy ***@***.***> wrote:  This seemed important and is a real problem but I haven't heard back from anyone? In the absence of confirmation I will assume my fix is good and will merge and release it later today — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[pehagg]

@mikehardy , sorry for not replying earlier. I've been away from my computer for the last three weeks. I'll have a look at this later today.

[mikehardy]

No problem, hopefully I nailed the issue, it's released already but we can always do a follow up PR if needed

[pehagg]

I'm planning to update our libraries next, will verify the fix within the next 24 hours or so. Got a little bit delayed by other stuff after returning from the vacays.

[pehagg]

Can confirm that the fix is working when using version 12.3.0.