atn832 / fake_cloud_firestore

BSD 2-Clause "Simplified" License
118 stars 90 forks source link

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

Open alexagat opened 3 years ago

alexagat commented 3 years ago

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}}
atn832 commented 3 years ago

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.

alexagat commented 3 years ago

@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},
      ],
    );
  });
}
danielmimimi commented 2 years ago

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

gmcdowell commented 1 year ago

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

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

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