Improve error message when passing circular data to query

[MichaelLeeHobbs]

Do you want to request a feature or report a bug? Bug

What is the current behavior? Maximum call stack size exceeded isBsonType.js:9

If the current behavior is a bug, please provide the steps to reproduce.
Calling a sequlize model.findAll(options) where options has an include array in a mongoose schema.statics.function will cause mongoose to throw a RangeError that is not catchable. I was able to confirm that even a simple include will cause the error to be thrown.

Example

    const options = {
        attributes: studyAttributes,
        include: [
            {model: exaDB.models.patients, attributes: patientAttributes},
        ],
        where: {has_deleted: false},
        offset,
        limit
    }

C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\isBsonType.js:9
function isBsonType(obj, typename) {
                   ^

RangeError: Maximum call stack size exceeded
    at isBsonType (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\isBsonType.js:9:20)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:74:7)
    at cloneArray (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:139:14)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:36:12)
    at cloneObject (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:120:17)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:59:16)
    at cloneObject (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:120:17)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:59:16)
    at cloneArray (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:139:14)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:36:12)
    at cloneObject (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:120:17)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:59:16)
    at cloneObject (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:120:17)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:59:16)
    at cloneArray (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:139:14)
    at clone (C:\Users\mhobb\WebstormProjects\vns\portal\portal-server\node_modules\mongoose\lib\helpers\clone.js:36:12)

What is the expected behavior? Not to throw and if throwing should be catchable.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
node v14.17.0
Mongoose: 5.12.14 also tested on 5.3
MongoDB: 4.4.6 and DocumentDB 4.0.0

getStudyFromExa

const exaDB = require('../../../exa/ExaDatabase')

const getStudyFromExa = async ({id, accession, offset = 0, limit = 100}) => {
    exaDB.models.studies.belongsTo(exaDB.models.patients, {foreignKey: 'patient_id', constraints: false})
    exaDB.models.studies.hasMany(exaDB.models.study_transcriptions, {foreignKey: 'study_id', constraints: false})
    exaDB.models.studies.hasOne(exaDB.models.peer_review, {foreignKey: 'study_id', constraints: false})
    exaDB.models.peer_review.belongsTo(exaDB.models.providers, {foreignKey: 'review_phys_id', as: 'reviewing', constraints: false})
    exaDB.models.study_transcriptions.belongsTo(exaDB.models.provider_contacts, {foreignKey: 'approving_provider_id', as: 'approving', constraints: false})
    exaDB.models.provider_contacts.belongsTo(exaDB.models.providers, {foreignKey: 'provider_id'})

    const studyAttributes = [
        'id', 'dicom_study_id', 'study_uid', 'accession_no', 'study_dt', 'study_description', 'body_part', 'department', 'reason_for_study', 'cpt_codes', 'stat_level', 'study_status',
        'study_info', 'notes', 'study_received_dt', 'study_details', 'modalities', 'priority', 'no_of_instances', 'institution', 'study_unread_dt', 'approved_dt'
    ]
    const patientAttributes = ['id', 'dicom_patient_id', 'account_no', 'alt_account_no', 'prefix_name', 'first_name', 'middle_name', 'last_name', 'suffix_name', 'gender', 'birth_date']
    const transcriptionsAttributes = ['id', 'study_id', 'transcription_data', 'created_dt', 'transcription_type', 'text_type', 'addendum_no', 'transcription_text', 'approved_dt']
    const reviewingAttributes = ['id', 'provider_code', 'first_name', 'middle_initial', 'last_name']
    const peerReviewAttributes = ['id', 'study_id', 'agree_id', 'agree_date', 'notes']
    const providerContactsAttributes = ['id', 'provider_id']
    console.log('options')
    const options = {
        attributes: studyAttributes,
        include: [
            {model: exaDB.models.patients, attributes: patientAttributes},
            {
                model: exaDB.models.study_transcriptions, attributes: transcriptionsAttributes, include: [{
                    model: exaDB.models.provider_contacts, as: 'approving', attributes: providerContactsAttributes,
                    include: [{model: exaDB.models.providers, attributes: reviewingAttributes}]
                }]
            },
            {model: exaDB.models.peer_review, attributes: peerReviewAttributes, include: [{model: exaDB.models.providers, as: 'reviewing', attributes: reviewingAttributes}]},
        ],
        where: {has_deleted: false},
        offset,
        limit
    }

    if (id) return await exaDB.models.studies.findByPk(id, options)
    if (accession) options.where.accession_no = accession
    try {
        console.log(options)
        // const studies = await exaDB.models.studies.findAll()     // this does not cause mongoose to throw
        const studies = await exaDB.models.studies.findAll(options) // this does cause mongoose to throw
        console.log(studies)
        return studies
    } catch (e) {
        console.error(e)
        throw new Error(e.message)
    }
}
module.exports = getStudyFromExa

Study Schema

schema.statics.exaSync = async function (accession, options) {
    debug('exaSync 1', accession)
    let studies = undefined
    // TODO this try/catch is temporary debugging remove and let caller handle
    try {
        studies = await getStudyFromExa({accession}) // running this line throws a RangeError that is not catchable
    } catch (e) {
        console.error(e)
        throw e
    }
    if (studies.length > 1) throw new Error(`More than one study returned for ${accession}!`)
    const exaStudyData = studies[0]
    debug('exaSync 2', exaStudyData.accession_no)
    let studyData = await reformatExaStudyData(exaStudyData)

    debug('exaSync 3', studyData.accession)
    let study = await _model.findOne({accession: studyData.accession})
    if (!study) return _model.create(studyData)
    study.update(studyData)
    return study.save
}

Edit: The getStudyFromExa given the same input in a unit test it returns the expected result.

[MichaelLeeHobbs]

We are using mongoose to store logs from sequlize. The log data returned from sequlize was circular when the includes are nested. Luckily, it's not an issue with moongose.

[vkarpov15]

Will re-open this to improve the error message in future releases.

[AbdelrahmanHafez]

We could resolve circular dependencies automatically, or maybe it could be an option in mongoose by using a solution similar to safe-json-stringify, or perhaps a plugin.

What do you think @vkarpov15?

[vkarpov15]

@AbdelrahmanHafez how do you resolve a circular dependency for a query? I don't think it makes much sense to execute a query for the string "[circular]" when there happens to be a circular object in the query. I think circular objects should throw an error, we just need to make the error more readable.

[AbdelrahmanHafez]

@vkarpov15
You're right, trying to resolve circular dependencies in queries wouldn't make much sense.
The reason I came across this issue is an error I stumbled upon while passing a value with circular dependency into an update object.
I'd say it makes sense to convert circular references in this case.
What do you think?

[vkarpov15]

How would you store a circular object in MongoDB though? AFAIK that's not possible.

[AbdelrahmanHafez]

On the top of my head, we'd convert any circular object to ''[circular *]. I'd say that's better than throwing an (uncatchable?) error.

[vkarpov15]

I think throwing a catchable error is a better approach because we can't store that structure in MongoDB. The current error isn't catchable because it's a stack overflow, but we can detect cycles and throw.

[AbdelrahmanHafez]

we can't store that structure in MongoDB
Why is that?

My use-case is storing thirdPartyServiceResult: Schema.Types.Mixed, which could be a simple { success: true, message: 'Added items successfully.' }, or it could be an error object that has a request and response with circular references.

It might not be a good idea to have that as the default behavior, but anyone who has a similar use case could take advantage of such behavior.

[vkarpov15]

If you convert a circular object to a string "[circular]", that means you're not storing the object itself. You can write a utility function that collapses circular references before passing them to Mongoose, but I think Mongoose defaulting to storing "[circular]" is a bit counterintuitive.