Skip to content

Commit

Permalink
fix(schema): handle _id: false in schema paths as a shortcut for se…
Browse files Browse the repository at this point in the history 
…tting the `_id` option to `false`

Fix #7480
  • Loading branch information
@vkarpov15
vkarpov15 committed Feb 8, 2019
1 parent ebff7ba commit 1b1acd5
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/schema.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,13 +388,18 @@ Schema.prototype.add = function add(obj, prefix) {
return;
}

if (obj._id === false) {
delete obj._id;
this.options._id = false;
}

prefix = prefix || '';
const keys = Object.keys(obj);

for (let i = 0; i < keys.length; ++i) {
const key = keys[i];

if (obj[key] == null) {
if (obj[key] == null || obj[key] === false) {

This comment has been minimized.

Sign in to view

Copy link
@abdielou

abdielou Feb 11, 2019

@vkarpov15 This introduces a break where, for example, we set a schema path with select: false

https://mongoosejs.com/docs/api.html#schematype_SchemaType-select

This comment has been minimized.

Sign in to view

Copy link
@vkarpov15

vkarpov15 Feb 13, 2019

Author Collaborator

@abdielou can you provide a more complete code sample?

This comment has been minimized.

Sign in to view

Copy link
@travist

travist Feb 13, 2019

We are also noticing a breaking change with this change as well. Our automated tests @ https://github.com/formio/formio are failing with the following diff.

https://monosnap.com/file/SfdvtLke4Eehxo4srIPz3PBeaBn28b

This is created with getObject() method from a nested schema that looks like this.

const schema = new mongoose.Schema({
  ...
  ...
  submissionAccess: [{
    _id: false,
    type: {
      type: String,
      enum: [
        'create_all',
        'read_all',
        'update_all',
        'delete_all',
        'create_own',
        'read_own',
        'update_own',
        'delete_own',
        'self'
      ],
      required: 'A permission type is required to associate an available permission with a given role.'
    },
    roles: {
      type: [mongoose.Schema.Types.ObjectId],
      ref: 'role'
    }
  }],
  ...
})

If you wish to run the tests to verify this... just do the following.

git clone git@github.com:formio/formio.git
cd formio
npm install
npm install mongoose@5.4.11
npm test

This comment has been minimized.

Sign in to view

Copy link
@vkarpov15

vkarpov15 Feb 13, 2019

Author Collaborator

@travist it looks like your tests are asserting that each submissionAccess has an _id, even though you specify _id: false in your schema. Is that intentional? Seems like you were getting bit by the bug we fixed in #7480

This comment has been minimized.

Sign in to view

Copy link
@travist

travist Feb 13, 2019

I was under the impression that a nested schema would not include an "_id" if the "_id: false" is provided. Maybe my assumption is wrong, but it does seem like you reverted this commit?

This comment has been minimized.

Sign in to view

Copy link
@ahce

ahce Feb 15, 2019

@vkarpov15 this commit breaks this logic:
exampleBreak is broken because _id false from otherProps causes the _id deletion of the schema. It only happens when otherProps is used as an object.

const mongoose = require('mongoose');

const otherProps = require('./otherProps');

/* otherProps
  {
    _id: false, // this breaks to exampleBreak
    prop1: { type: String },
    prop2: { type: String },
    prop3: { type: String }
  }
*/

const exampleWorks = new mongoose.Schema(
  {
    name: {
      type: String
    },
    data: [
      otherProps
    ]
  },
  {
    timestamps: true
  }
);

const exampleBreak = new mongoose.Schema(
  {
    name: {
      type: String
    },
    data: otherProps
  },
  {
    timestamps: true
  }
);

mongoose.model('exampleWorks', exampleWorks);
mongoose.model('exampleBreak', exampleBreak);

This comment has been minimized.

Sign in to view

Copy link
@vkarpov15

vkarpov15 Feb 18, 2019

Author Collaborator

@travist I did not fully revert this commit, we got rid of the code that threw an error if you tried to define a schema path as false: 5b0d78a . However, we kept the fix that makes new Schema({ _id: false }) work as intended. Before 5.4.11, new Schema({ _id: false }) was equivalent to new Schema({ _id: Mixed }), but now it is equivalent to new Schema({}, { _id: false }).

@ahce I don't understand what exactly is broken in your script. The below script executes without error for me. Please open up a new issue and follow the issue template.

const mongoose = require('mongoose');

const otherProps = {
    _id: false, 
    prop1: { type: String },
    prop2: { type: String },
    prop3: { type: String }
  }


const exampleWorks = new mongoose.Schema(
  {
    name: {
      type: String
    },
    data: [
      otherProps
    ]
  },
  {
    timestamps: true
  }
);

const exampleBreak = new mongoose.Schema(
  {
    name: {
      type: String
    },
    data: otherProps
  },
  {
    timestamps: true
  }
);

mongoose.model('exampleWorks', exampleWorks);
mongoose.model('exampleBreak', exampleBreak);

This comment has been minimized.

Sign in to view

Copy link
@ahce

ahce Feb 19, 2019

@vkarpov15 Thanks for your reply.
The issue, I updated script there.
throw new TypeError('Invalid value for schema path `' + prefix + key + '`');
}

Expand Down

0 comments on commit 1b1acd5

Please sign in to comment.