Firebase Functions Encoding Failure for Self-Referencing Objects Leads to Maximum call stack size exceeded

[DavidWeiss2]

Related issues
#737 #776 #844

Related Pull Request
#1525

Related Stack overflow questions:
https://stackoverflow.com/search?q=%5Bfirebase-functions%5D+Maximum+call+stack+size+exceeded

[REQUIRED] Version info
node: all versions of node, tested in 18

firebase-functions: all

firebase-tools: irrelevant

firebase-admin: irrelevant

[REQUIRED] Test case
A class instance with self-referencing properties causes a "Maximum call stack size exceeded" error when encoded. Example class:

class TestClass {
  foo: string;
  bar: number;
  self: TestClass;
  constructor(foo: string, bar: number) {
    this.foo = foo;
    this.bar = bar;
    this.self = this;
  }
}
const functions = require('firebase-functions');
exports.failingCode = functions.https.onCall((data, context) => {
   return new TestClass("hello",1);
});

[REQUIRED] Steps to reproduce
Define a class with a self-referencing property.
Create an instance of this class.
Attempt to return the object

[REQUIRED] Expected behavior
The object should be encoded without resulting in a stack overflow, properly handling self-references within the object structure.

[REQUIRED] Actual behavior
Encoding such an object leads to a firebase on call /workspace/node_modules/firebase-functions/lib/common/providers/https.js "Maximum call stack size exceeded" error, indicating an unhandled recursive loop in the encoding process.

Were you able to successfully deploy your functions?
Yes, but the function execution fails when encountering objects with self-references.

EDIT:
actual code from repo with the error (simplified):

const functions = require('firebase-functions');
exports.getAllSuperAdmins = functions.https.onCall((data, context) => {
       const totalAdmins: DbUserModel[] = [];

    for await (const users of firestoreUsersPaginator([Filter.where('isAdmin', '==', true)])) {
      logger.log('get all super admins', users.length);
      // checking that the users have the right claims and update docs without the right claims
      totalAdmins.push(...(await filterAndFixUsersWithInvalidData(users))); 
      logger.log('totalAdmins', totalAdmins.length);
    }
    
    return totalAdmins
});

async function* firestoreUsersPaginator(customQuery: Filter[] = [], pageSize: number = 1000) {
  pageSize = Math.min(pageSize, 1000); // max page size is 1000
  const baseQuery = admin
    .firestore()
    .collection('users') as firestore.CollectionReference<DbUserModel>;
  let query: FirebaseFirestore.Query<DbUserModel> = baseQuery;

  for (const q of customQuery) {
    query = query.where(q);
  }

  query = query.orderBy('__name__').limit(pageSize);

  let pageLastDoc: firestore.QueryDocumentSnapshot<DbUserModel> | undefined = undefined;
  let i = 0;
  while (true) {
    const pageQuery: firestore.Query<DbUserModel> = pageLastDoc
      ? query.startAfter(pageLastDoc.id)
      : query;

    const usersDocs = await pageQuery.get();
   // pagination logic
  }
  return 'OK';
}

[DavidWeiss2]

@inlined Can someone from the firebase team look into it please? we faced this issue twice from Firestore doc data objects on a list return from a collection.

[DavidWeiss2]

Bump

[DavidWeiss2]

@inlined @kirstywilliams

[inlined]

Sorry for the delay. I don't think there's a reasonable platform-wide solution to resolving circular links in a JSON-based API. While loggers can replace a circular reference with a sentinel with little risk of breaking an application, modifying a customer's response object seems like a dangerous path to breaking clients. IMO the appropriate solution here is for your class to implement toJSON to return a JSON serializeable object. Here's a modified version of your repro that would work.

[DavidWeiss2]

@inlined,

Thanks for your feedback. Your concerns about changing user response objects are valid. The issue I've encountered affects more than logging; it occurs during actual API responses, notably in authentication processes, and the source of the error was an object I got from the firebase auth lib, on error of Maximum call stack you cannot know what caused the error in your app, because of lacing in error context in trace history. This making it hard to diagnose.

My PR #1525 offers a non-intrusive fix that preserves the integrity of user objects while addressing circular references, enhancing error clarity without altering or removing object properties.

Please review PR #1525 for the technical specifics. I believe it aligns with our goal of improving developer experience without compromising object integrity.

Looking forward to your thoughts.

[DavidWeiss2]

I adding to the Issue a ref to the code that was acutely in my repo to make it clearer.

[DavidWeiss2]

@inlined ?

[inlined]

Hmm... the issue being closed hid it from my bug inbox, but I found the GitHub alert. Reopening to make sure this gets tracked.

Thanks for doing some good legwork for us. I still have a lot of hesitation, but I can bring this up with the team and see if there's room for compromise. There's three behaviors that are possible:

1. Error on circular references (current): Chosen default based on the principle that dead programs tell no lies.
2. Omit circular references entirely: A close cousin to (1) that refuses to partially handle circular references.
3. Deep copy circular references without the back-ref (proposed): A more magical alternative to (2) that might support more use cases, but fails in more subtle ways. It also technically costs the users more money because it increases response size and response egress is billed.

Would 2 be a reasonable compromise in your case?

If we do change behavior, we should try to find ways to reduce the odds that we get support requests if devs are really trying to do something unsupported (e.g. customers may expect that their JS web app can directly decode JS objects sent via encode). Options here are to:

1. Emit a warning every time a circular reference is dropped. This decreases the odds of a support case, but we might need to throttle log lines to find the right balance between making sure the user sees the log and flooding the log stream and making it useless.
2. Add a new function that makes it clear this opt-in behavior is happening (e.g. encodeDroppingCircular). This will require me to file a formal API proposal internally, which I'm happy to do but will add some time before we can commit the change.

I have moderate preference for 2, but I'm curious to hear what you think.

[inlined]

Additionally, can you explain what specifically is the circular ref in your example? It looks like you're possibly trying to return a QuerySnapshot or QueryDocumentSnapshot? You won't be able to deserialize them in the client even if you dropped the circular references; you'd want to call QueryDocumentSnapshot.data() to get a JSON serializeable object.

[DavidWeiss2]

@inlined,

Thank you for reconsidering this issue and for the detailed exploration of potential solutions. I greatly appreciate the effort to find a compromise that balances developer needs with system integrity.

I agree with the hesitation regarding the automatic deep copying of circular references due to the potential for subtle failures and the increased cost implications. The option to emit a warning when circular references are dropped resonates with me as a balanced approach. It informs developers of the modification while maintaining the system's integrity and avoiding the complexities of deep copying.

Introducing a global option and a per-usage override for handling circular references offers the flexibility needed for various use cases. This approach allows developers to make informed decisions based on their specific requirements and contexts, which I believe is a valuable enhancement to the system.

I'm supportive of your preference for option 2, with a focus on warning developers about dropped circular references. This seems to be a pragmatic solution that enhances developer awareness without overly complicating the system.

I'm looking forward to seeing how these ideas evolve and am happy to provide further input or assist in testing potential implementations.

I would love to be part of this changes once the team decided on how to approach this.

Regarding the circular reference in my example, I suspect it originated from referencing a document in another database. Unfortunately, due to the nature of the error and lack of detailed trace context, I was unable to conclusively verify this. The inability to easily identify the source of circular references in complex objects underscores the importance of enhancing error reporting and handling mechanisms.

[jhuleatt]

@DavidWeiss2 thanks for your thoughtful feedback.

I vote for option 1 from @inlined's list. The docs are clear about requiring a JSON-serializable object:

To send data back to the client, return data that can be JSON encoded.

Because of that, I'd consider anything that would error in a JSON.stringify call to be an invalid response. For any object that can't be serialized by default, I'd expect toJSON to be respected.

The cryptic error message here could be considered to be a bug, though. @inlined, would it make sense to do a quick check in encode to make sure the object is serializable, and throw a clearer error if not? Something like this:

function encode(data) {
  // ...
  try {
    JSON.stringify(data);
  } catch (e) {
    throw new Error("Data cannot be converted to JSON");
  }
  // ...do the real encoding
}

[DavidWeiss2]

@jhuleatt right now the encode is stripping functions from the object,I don't see any reason to start failing on tha scanrio of json.strigify fail.

[inlined]

After consulting with the team we've decided to keep the function behavior as-is, since it is documented as a JSON API. You can use the method you've coded up already locally to create a JSON object that you then return in your callable function.

[DavidWeiss2]

Ok, I am fixing the PR to reflect this decision so the user will get a clear error on where the error occurred and in which object.

[DavidWeiss2]

#1525 Been changed to reflect this decision.