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

Unable to filter response from 'find / findOne' within plugin #11426

Closed
a3957273 opened this issue Feb 18, 2022 · 9 comments
Closed

Unable to filter response from 'find / findOne' within plugin #11426

a3957273 opened this issue Feb 18, 2022 · 9 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@a3957273
Copy link

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

What is the current behavior?
Unable to filter response from a .find() or .findOne() call.

If the current behavior is a bug, please provide the steps to reproduce.
Given a plugin:

(schema: Schema) => {
    schema.post('findOne', async function(doc) {
      if (await shouldRemove(doc)) return undefined
    
      return doc
    })
  }

I should be able to selectively return either nothing, or a document, from .find() and .findOne() calls.

What is the expected behavior?
If shouldRemove returns true, for no document to be returned from undefined.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
Mongoose v6.1.6, Node v16.13.2,

@Uzlopak
Copy link
Collaborator

Uzlopak commented Feb 18, 2022

Can you provide an mvce please?

@IslandRhythms IslandRhythms added the new feature This change adds new functionality, like a new method or class label Feb 18, 2022
@a3957273
Copy link
Author

a3957273 commented Feb 21, 2022

EDIT: This code has some typos in it, check out the code two comments below for an example that runs.

import { model, Mongoose } from 'mongoose'

const ExampleSchema = newSchema({
  hidden: Boolean
})

async function shouldRemove(doc) {
  return doc.hidden
}

ExampleSchema.plugin((schema) => {
  schema.post('findOne', async function(doc) {
    if (await shouldRemove(doc)) return undefined
  
    return doc
  })
})

const ExampleModel = model('Example', ExampleSchema)

Mongoose.connect('<SOME_MONGOOSE_INSTANCE')

async function main() {
  await ExampleModel.create({ hidden: true })

  const empty = await ExampleModel.findOne({ hidden: true })
  // empty = undefined

  await ExampleModel.create({ hidden: false })
  
  const found = await ExampleModel.findOne({ hidden: false })
  // found = Doc<{ hidden: false }>
}

main()

@a3957273
Copy link
Author

a3957273 commented Feb 21, 2022

The example is a little contrived / simplified. Here we could add { hidden: false } to the database query in a schema.pre('findOne') section. This isn't possible in our real code due to our checks requiring complex JavaScript that cannot be converted to a database query.

@vkarpov15 vkarpov15 added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue and removed new feature This change adds new functionality, like a new method or class labels Feb 25, 2022
@vkarpov15 vkarpov15 added this to the 6.2.5 milestone Feb 25, 2022
@IslandRhythms IslandRhythms added can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. labels Mar 2, 2022
@a3957273
Copy link
Author

a3957273 commented Mar 3, 2022

Hey @IslandRhythms, you added 'confirmed-bug', is this meant to be an existing feature? Also, here's some functional reproduction code:

import mongoose, { model, Schema } from 'mongoose'

const ExampleSchema = new Schema({
  hidden: Boolean
})

async function shouldRemove(doc) {
  return doc.hidden
}

ExampleSchema.plugin((schema) => {
  schema.post('findOne', async function(doc) {
    if (await shouldRemove(doc)) return undefined
  
    return doc
  })
})

const ExampleModel = model('Example', ExampleSchema)
mongoose.connect('mongodb://localhost:27017/bailo')

async function main() {
  await ExampleModel.create({ hidden: true })

  const empty = await ExampleModel.findOne({ hidden: true })
  console.log('empty', empty)
  // empty = undefined

  await ExampleModel.create({ hidden: false })
  
  const found = await ExampleModel.findOne({ hidden: false })
  console.log('found', found)
  // found = Doc<{ hidden: false }>
}

main()

The 'empty' log should be empty, but it isn't!

@vkarpov15 vkarpov15 modified the milestones: 6.2.5, 6.2.6 Mar 9, 2022
@vkarpov15 vkarpov15 removed the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label Mar 9, 2022
@a3957273
Copy link
Author

Do you not see this issue as happening @vkarpov15? The latest script should clearly show a lack of ability to remove a model from the results of findOne / find.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Mar 10, 2022

It is not a bug but a feature request.

@a3957273
Copy link
Author

Ah, fair fair. Would you accept outside contributions? Could you point out what would need to be done to implement this?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Mar 10, 2022

@a3957273

Sure. I am not an official maintainer and I kind of spam the project with PRs :).

I dont know where precisely it needs to be patched. But the hooks are managed by the npm package named "kareem". So I would look into the files where kareem is required.

@vkarpov15
Copy link
Collaborator

I'm sorry it took me so long to get to this. We'll think more about how best to support this, but we'll have to add support in a minor release.

First, worth noting: you can modify the result of find() in post hooks, you just need to modify the array in place using splice()

ExampleSchema.plugin((schema) => {
  schema.post('find', async function(docs) {
    for (let i = docs.length - 1; i >= 0; --i) {
      if (await shouldRemove(docs[i])) {
        docs.splice(i, 1);
      }
    }
  })
})

A few notes about couple of issues to work around for implementing this:

  1. We can't support return undefined because that would be backwards breaking, no way to tell the difference between return undefined and no return statement. Have to do this instead:
ExampleSchema.plugin((schema) => {
  schema.post('findOne', async function(doc) {
    if (await shouldRemove(doc)) return null
  
    return doc
  })
})
  1. Allowing returning arbitrary values from post hooks would be a bit messy for TypeScript integrations. There's a workaround for developers because we support overriding the return type using Model.findOne<CustomType>(), but there isn't a good way for us to infer the findOne() return type at compile time if we support returning arbitrary values.

@vkarpov15 vkarpov15 modified the milestones: 6.2.8, 6.x Unprioritized Mar 23, 2022
@vkarpov15 vkarpov15 added the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Mar 23, 2022
@vkarpov15 vkarpov15 reopened this May 7, 2022
@vkarpov15 vkarpov15 removed this from the 6.x Unprioritized milestone May 7, 2022
@vkarpov15 vkarpov15 added this to the 6.4 milestone May 7, 2022
vkarpov15 added a commit to mongoosejs/kareem that referenced this issue May 29, 2022
vkarpov15 added a commit that referenced this issue Jun 13, 2022
…` for skipping and modifying middleware results

Fix #11426
Fix #8393
vkarpov15 added a commit that referenced this issue Jun 17, 2022
feat: add `skipMiddlewareFunction()` and `overwriteMiddlewareResult()` for skipping and modifying middleware results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

4 participants