Skip to content
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

Error when persisting nested objects #143

Closed
lfernando-silva opened this issue Feb 22, 2018 · 14 comments
Closed

Error when persisting nested objects #143

lfernando-silva opened this issue Feb 22, 2018 · 14 comments
Assignees
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. 🚨 This issue needs some love. triage me I really want to be triaged. web

Comments

@lfernando-silva
Copy link

Environment details

  • OS: Linux Mint
  • Node.js version: 8.9.4 LTS
  • npm version: 5.6.0
  • @google-cloud/firestore version: 0.11.2

Steps to reproduce

  1. call function ref.doc(id).set(params, options), which params is a object like:
{
  "notifications": {
    "web": "a random token string"
  },
  "updatedAt": 1519320011313
}

Expected behavior

  • Persist the object

Actual behavior

  • I got error Cannot encode type ([object Object]) to a Firestore Value. The [object Object] is the nested object ( {web : a random token string} ).

I realized the function isPlainObject at line 1428 returns true for {notifications...}, but returns false for the nested object {web: ....}, as it has no Object.prototype property.

//firestore/src/document.js 1428

function isPlainObject(input) {
  return (
    typeof input === 'object' &&
    input !== null &&
    Object.getPrototypeOf(input) === Object.prototype
  );
}

When I tried to force input to be an object

function isPlainObject(input) {
  return (
    typeof input === 'object' &&
    input !== null &&
(Object.getPrototypeOf(input) === Object.prototype) || (Object.getPrototypeOf(Object.create(input)) === Object.prototype)
  );
}

I got another error for the other value at not nested object:
Argument \"data\" is not a valid Document. Object prototype may only be an Object or null: 1519320772620.

Using lodash 's isObject method:

function isPlainObject(input) {
    return require('lodash').isObject(input) //returns true for both objects
}

I got the error
Argument \"data\" is not a valid Document. obj.hasOwnProperty is not a function

How can I handle this problem? Thank you in advance.

@ghost
Copy link

ghost commented Feb 23, 2018

I had the same problem. My solution was to convert the nested objects to plain objects using lodash before sending them to Firestore. For example, instead of something like this:

let params = new ParamsObject();
params. notifications = new WebObject();

I convert the objects to plain objects using lodash before sending them to Firestore.

let params = _.toPlainObject(new ParamsObject());
params. notifications = _.toPlainObject(new WebObject());

This works, but if there is a better way to solve this problem, I would also like to know.

@schmidt-sebastian
Copy link
Contributor

The following code snippet works for me:

let ref = randomCol.doc('doc1');
    return ref
      .set({
        notifications: {
          web: 'a random token string',
        },
        updatedAt: 1519320011313,
      })
      .then(() => ref.get())
      .then(snap => {
        console.log(snap.data());
      });

Are you using a class to encapsulate your data? Please note that we explicitly decided to not support serialization of custom classes for the Web and Node.JS client as we won't be able to deserialize back into these types.

I will bring this up with our team for discussion and see if we can make this easier for our users. For now, the easiest (and least performant) way to work around this issue is by using JSON.stringify() and JSON.parse().

@ghost
Copy link

ghost commented Feb 27, 2018

In my case I was using classes to encapsulate my data. I received the same error as @lfernando-silva ( Cannot encode type ([object Object]) to a Firestore Value) which was a little confusing, since the class instance is technically an object. I then found the isPlainObject test in the firestore client source and only then realised that I could not pass a class instance to the Firestore client directly, and that I should first convert my data to a plain object. It would be nice if this decision was better documented, maybe something like the description on what queries are valid or invalid here.

@schmidt-sebastian
Copy link
Contributor

schmidt-sebastian commented Feb 27, 2018

The v0.12.0 release we pushed yesterday added some additional input validation that improved the error message a little. The error for a set() call is now: 'Argument "data" is not a valid Document. Cannot use custom type "Foo" as a Firestore type.'

We can certainly look at improving this error further and at improving our documentation.

@lfernando-silva
Copy link
Author

lfernando-silva commented Feb 27, 2018

Seems it not was published yet at nodejs firebase-admin on npm.

@colombotapps , in my case, it was just an object passed by param (as you said, actually classes and object are the same at js), as it cames from an react app as request. Really it can be an not plain object.

I didn't try to parse the nested object and other stuff because I'am work on other project, but its totally makes sense.

@schmidt-sebastian
Copy link
Contributor

Firebase Admin will pull the new version of Firestore with its next release (firebase/firebase-admin-node#216).

@schmidt-sebastian
Copy link
Contributor

We have now improved the error message further, and will push a new release containing this fix soon. You will now see errors such as Couldn't serialize object of type "Foo". Firestore doesn't support JavaScript objects with custom prototypes (i.e. objects that were created via the 'new' operator).

As for the underlying reasoning, please also take a look at firebase/firebase-js-sdk#476

@ronnieroyston
Copy link

Mistakenly leave .doc() off of the Firestore reference passed to .set and the error message is :

Argument "documentRef" is not a valid DocumentReference. Couldn't serialize object of type "CollectionReference". Firestore doesn't support JavaScript objects with custom prototypes (i.e. objects that were created via the 'new' operator).

Suggestion, have the .set method detect this particular case and throw a more helpful message such as :

Firestore .set method requires an ID for the document to create. If you want Firestore to auto-generate an ID, add .doc to your reference.

@BkunS
Copy link

BkunS commented Jan 8, 2019

Had the issue working with GraphQL (resolver), fixed it by

data = JSON.parse(JSON.stringify(data));

GraphQL does pass the data in as an object, but somehow there might be something missing inside the object, causing the object failed to pass firestore validation. Re-creating the object might hurt the performance a little, but that's the only way I find out to solve it.

@google-cloud-label-sync google-cloud-label-sync bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Jan 31, 2020
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Apr 6, 2020
@manwithsteelnerves
Copy link

if we need to use JSON.parse to fix this issue, how can we make use of setting FieldValues like Increment and Array Union operations?

@schmidt-sebastian
Copy link
Contributor

If you can provide a reproduction where our check fails (like the one mentioned above), we can take another look.

@manwithsteelnerves
Copy link

manwithsteelnerves commented Jun 26, 2020

class User {
  name : string = 'default name';
 statistics : Statistics = new Statistics();
}

class Statistics {
 likes : number = 0;
updatedAt! : ServerTimestamp;
}

When we need to update the user's likes, we try to use atomic increment operator.

const user : User = <User>{};
user.statistics = new Statistics();
user.likes = <any>admin.firestore.FieldValue.increment(1); //Just to make use of the atomic increment feature

updateUser(user); //This saves to firestore (in UpdateUser, we consider converting the keys to dot notation)

If updateUser uses json conversion, the new operator error goes away. But FieldValue class from increment or timestamp getting converted by to text instead of actually doing the operation.

Here user.likes will be stored as

likes : {
  operand : 1
}

Hope I'm clear and not missing any.

@schmidt-sebastian
Copy link
Contributor

@manwithsteelnerves The code snippet you provided shows a pattern that is explicitly not supported. We cannot roundtrip the User type without losing its type (e.g. prototype) information. If we were to allow this, the data you write to Firestore will not match the data that is read from Firestore.

If you want to persist custom Objects, you have to use withConverter() and strip all prototype information manually. The converter can then be used to reconstruct the original object: https://github.com/googleapis/nodejs-firestore/blob/56355f1/types/firestore.d.ts#L47

@manwithsteelnerves
Copy link

manwithsteelnerves commented Jun 26, 2020

I just went through the reason shared earlier why its not supported. That makes total sense.
Here I don't want to use explicit dot notation string and direct firestore methods to update. This is because its difficult to maintain if I change a variable name.

However, I was able to solve it with some additional steps.

In updateUser method

  1. Convert the data with JSON.parse(JSON.stringify(data))
  2. Run a for loop for each key of data and check if its of type FieldValue
  3. If its field value, I update the parsed data from step 1 to the field value
public async updateDocument<T>(resourcePath : string, data : {[fieldPath: string]: any }, type : { new(): T }) : Promise<void>
  {
    const start = Date.now();

    const documentRef   : DocumentReference   = getDatabase().doc(resourcePath);

    // Updating timestamp
    data._updatedAt = Date.now();

    const jsonData = JSON.parse(JSON.stringify(data));
    Object.keys(data).forEach(key => {
      if(data[key] instanceof FieldValue) {
          jsonData[key] = data[key]; //Resetting the values back as field value class instances are allowed custom/special class objects
      }
    });

    await documentRef.update(jsonData);
    
    console.log(`[Time : ${Date.now() - start} ms] Update Document : ${resourcePath}`);
  }

Also, as you notice, its the problem with the firestore FieldValue class values as its a special operation and may be processed differently internally. As mentioned, I don't want to miss the type information so can't go with direct firestore updates (similar to examples of dot notation shown over here. They totally aren't type safe - favorites.color)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. 🚨 This issue needs some love. triage me I really want to be triaged. web
Projects
None yet
Development

No branches or pull requests

7 participants