fake_cloud_firestore icon indicating copy to clipboard operation
fake_cloud_firestore copied to clipboard

`doc.update` not properly overwriting map value, and instead merging

Open alexagat opened this issue 3 years ago • 5 comments

The update function is not following the same behavior as the true cloud_firestore package.

The following example illustrates the differences in result. In both examples assume the doc exists with an initial value of:

'testMap': {'a': 1}

Using cloud_firestore

doc.update('testMap': {'b': 2});

// result is {'testMap': {'b': 2}}

Using cloud_firestore_mocks

doc.update('testMap': {'b': 2});

// result is {'testMap': {'a': 1, 'b': 2}}

alexagat avatar Apr 24 '21 18:04 alexagat

Oh that's strange. The official doc says:

Updates data on the document. Data will be merged with any existing document data. If no document exists yet, the update will fail.

https://pub.dev/documentation/cloud_firestore/latest/cloud_firestore/DocumentReference/update.html

I'll check manually later.

atn832 avatar May 20 '21 23:05 atn832

@atn832 if it helps here are some unit tests that show 2 cases of failure compared with the result from the cloud_firestore API. The first test is the same as this GH issue, where as the second shows a failure to merge equal objects with the arrayUnion fieldValue.

/*
Due to this issue: https://github.com/atn832/cloud_firestore_mocks/issues/162
The cloud_firestore_mocks package does not properly overwriting map value
and instead merges the values. This means we can not make proper
assertions on our firestore update calls.

The below illustrates the bug.
*/

import 'package:cloud_firestore/cloud_firestore.dart';
import 'package:cloud_firestore_mocks/cloud_firestore_mocks.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:study/core/models/error.dart';

class CloudFirestoreMocksTest {
  final FirebaseFirestore db;

  CloudFirestoreMocksTest(this.db);

  DocumentReference get documentReference => db.collection('users').doc('u1');

  void testUpdate(Map<String, String> map) {
    try {
      documentReference.update({'testMap': map});
    } catch (error) {
      throw AppError('Unable to update');
    }
  }

  void testArrayUnion(String field, dynamic newValue) {
    Map<String, dynamic> data = {};
    data[field] = FieldValue.arrayUnion([newValue]);
    try {
      documentReference.set(
        data,
        SetOptions(merge: true),
      );
    } catch (error) {
      throw AppError('Unable to set array union');
    }
  }
}

main() {
  test('test update', () async {
    final testSubject = CloudFirestoreMocksTest(MockFirestoreInstance());

    await testSubject.db.collection('users').doc('u1').set({
      'testMap': {'a': '1'},
    });

    testSubject.testUpdate({'b': '2'});

    var doc = await testSubject.db.collection('users').doc('u1').get();

    expect(doc.data()['testMap'], {'b': '2'});
  });

  test('test array union of objects with merge', () async {
    final testSubject = CloudFirestoreMocksTest(MockFirestoreInstance());

    await testSubject.db.collection('users').doc('u1').set({
      'followers': [
        {'name': 'Alex', 'uid': 1},
        {'name': 'Bob', 'uid': 2},
        {'name': 'Mary', 'uid': 3},
      ],
    });

    testSubject.testArrayUnion('followers', {'name': 'Bob', 'uid': 2});

    var doc = await testSubject.db.collection('users').doc('u1').get();

    expect(
      doc.data()['followers'],
      [
        {'name': 'Alex', 'uid': 1},
        {'name': 'Bob', 'uid': 2},
        {'name': 'Mary', 'uid': 3},
      ],
    );
  });
}

alexagat avatar May 23 '21 01:05 alexagat

I faced similar issues, the update feature cannot be testet properly since it will just set the data.

danielmimimi avatar Nov 27 '21 16:11 danielmimimi

I think this issue almost manifests when trying to 'remove' a MapEntry 'testMap': {'a': 1}

doc.update('testMap': {});
// result is {'a': 1}`

gmcdowell avatar Mar 24 '23 04:03 gmcdowell

I also encountered this issue while writing tests. Is there any update if and when it will be changed/fixed?

timdebruyn avatar Jun 26 '23 09:06 timdebruyn