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

feat(model): add create option "immediateError" #13544

Merged
merged 5 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 37 additions & 15 deletions lib/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,6 @@ function generateVersionError(doc, modifiedPaths) {
* newProduct === product; // true
*
* @param {Object} [options] options optional options
* @param {Boolean} [options.ordered] saves the docs in series rather than parallel.
* @param {Session} [options.session=null] the [session](https://www.mongodb.com/docs/manual/reference/server-sessions/) associated with this save operation. If not specified, defaults to the [document's associated session](https://mongoosejs.com/docs/api/document.html#Document.prototype.session()).
* @param {Object} [options.safe] (DEPRECATED) overrides [schema's safe option](https://mongoosejs.com/docs/guide.html#safe). Use the `w` option instead.
* @param {Boolean} [options.validateBeforeSave] set to false to save without validating.
Expand Down Expand Up @@ -2802,6 +2801,8 @@ Model.findByIdAndRemove = function(id, options) {
*
* @param {Array|Object} docs Documents to insert, as a spread or array
* @param {Object} [options] Options passed down to `save()`. To specify `options`, `docs` **must** be an array, not a spread. See [Model.save](https://mongoosejs.com/docs/api/model.html#Model.prototype.save()) for available options.
* @param {Boolean} [options.ordered] saves the docs in series rather than parallel.
* @param {Boolean} [options.aggregateErrors] Aggregate Errors instead of throwing the first one that occurs. Default: false
* @return {Promise}
* @api public
*/
Expand Down Expand Up @@ -2858,27 +2859,41 @@ Model.create = async function create(doc, options) {
return Array.isArray(doc) ? [] : null;
}
let res = [];
const immediateError = typeof options.aggregateErrors === 'boolean' ? !options.aggregateErrors : true;

delete options.aggregateErrors; // dont pass on the option to "$save"

if (options.ordered) {
for (let i = 0; i < args.length; i++) {
const doc = args[i];
const Model = this.discriminators && doc[discriminatorKey] != null ?
this.discriminators[doc[discriminatorKey]] || getDiscriminatorByValue(this.discriminators, doc[discriminatorKey]) :
this;
if (Model == null) {
throw new MongooseError(`Discriminator "${doc[discriminatorKey]}" not ` +
try {
const doc = args[i];
const Model = this.discriminators && doc[discriminatorKey] != null ?
this.discriminators[doc[discriminatorKey]] || getDiscriminatorByValue(this.discriminators, doc[discriminatorKey]) :
this;
if (Model == null) {
throw new MongooseError(`Discriminator "${doc[discriminatorKey]}" not ` +
`found for model "${this.modelName}"`);
}
let toSave = doc;
if (!(toSave instanceof Model)) {
toSave = new Model(toSave);
}
}
let toSave = doc;
if (!(toSave instanceof Model)) {
toSave = new Model(toSave);
}

await toSave.$save(options);
res.push(toSave);
await toSave.$save(options);
res.push(toSave);
} catch (err) {
if (!immediateError) {
res.push(err);
} else {
throw err;
}
}
}
return res;
} else {
res = await Promise.all(args.map(async doc => {
// ".bind(Promise)" is required, otherwise results in "TypeError: Promise.allSettled called on non-object"
const promiseType = !immediateError ? Promise.allSettled.bind(Promise) : Promise.all.bind(Promise);
let p = promiseType(args.map(async doc => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this be just Promise.allSettled(args.map(...)) ? bind() is relatively slow, so I would prefer to avoid using that.

Copy link
Collaborator Author

@hasezoey hasezoey Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had avoided that because using allSettled in all cases would mean to both wait until all operations have finished, only to then throw the first error and second the allSettled result has to be mapped because of what it returns (ie to keep the current behavior unless requested)

const Model = this.discriminators && doc[discriminatorKey] != null ?
this.discriminators[doc[discriminatorKey]] || getDiscriminatorByValue(this.discriminators, doc[discriminatorKey]) :
this;
Expand All @@ -2896,6 +2911,13 @@ Model.create = async function create(doc, options) {

return toSave;
}));

// chain the mapper, only if "allSettled" is used
if (!immediateError) {
p = p.then(presult => presult.map(v => v.status === 'fulfilled' ? v.value : v.reason));
}

res = await p;
}


Expand Down
46 changes: 46 additions & 0 deletions test/model.create.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,52 @@ describe('model', function() {
const docs = await Test.find();
assert.equal(docs.length, 5);
});

it('should return the first error immediately if "aggregateErrors" is not explicitly set (ordered)', async function() {
const testSchema = new Schema({ name: { type: String, required: true } });

const TestModel = db.model('gh1731-1', testSchema);

const res = await TestModel.create([{ name: 'test' }, {}, { name: 'another' }], { ordered: true }).then(null).catch(err => err);

assert.ok(res instanceof mongoose.Error.ValidationError);
});

it('should not return errors immediately if "aggregateErrors" is "true" (ordered)', async function() {
const testSchema = new Schema({ name: { type: String, required: true } });

const TestModel = db.model('gh1731-2', testSchema);

const res = await TestModel.create([{ name: 'test' }, {}, { name: 'another' }], { ordered: true, aggregateErrors: true });

assert.equal(res.length, 3);
assert.ok(res[0] instanceof mongoose.Document);
assert.ok(res[1] instanceof mongoose.Error.ValidationError);
assert.ok(res[2] instanceof mongoose.Document);
});
});

it('should return the first error immediately if "aggregateErrors" is not explicitly set', async function() {
const testSchema = new Schema({ name: { type: String, required: true } });

const TestModel = db.model('gh1731-3', testSchema);

const res = await TestModel.create([{ name: 'test' }, {}, { name: 'another' }], {}).then(null).catch(err => err);

assert.ok(res instanceof mongoose.Error.ValidationError);
});

it('should not return errors immediately if "aggregateErrors" is "true"', async function() {
const testSchema = new Schema({ name: { type: String, required: true } });

const TestModel = db.model('gh1731-4', testSchema);

const res = await TestModel.create([{ name: 'test' }, {}, { name: 'another' }], { aggregateErrors: true });

assert.equal(res.length, 3);
assert.ok(res[0] instanceof mongoose.Document);
assert.ok(res[1] instanceof mongoose.Error.ValidationError);
assert.ok(res[2] instanceof mongoose.Document);
});
});
});
9 changes: 8 additions & 1 deletion test/types/create.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Schema, model, Types, CallbackError } from 'mongoose';
import { Schema, model, Types, HydratedDocument } from 'mongoose';
import { expectError, expectType } from 'tsd';

const schema = new Schema({ name: { type: 'String' } });
Expand Down Expand Up @@ -118,3 +118,10 @@ Test.insertMany({ _id: new Types.ObjectId('000000000000000000000000'), name: 'te
(await Test.create([{ name: 'test' }]))[0];
(await Test.create({ name: 'test' }))._id;
})();

async function createWithAggregateErrors() {
expectType<(HydratedDocument<ITest>)[]>(await Test.create([{}]));
expectType<(HydratedDocument<ITest> | Error)[]>(await Test.create([{}], { aggregateErrors: true }));
}

createWithAggregateErrors();
9 changes: 7 additions & 2 deletions types/models.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ declare module 'mongoose' {
SessionOption {
checkKeys?: boolean;
j?: boolean;
ordered?: boolean;
safe?: boolean | WriteConcern;
timestamps?: boolean | QueryTimestampsConfig;
validateBeforeSave?: boolean;
Expand All @@ -132,6 +131,11 @@ declare module 'mongoose' {
wtimeout?: number;
}

interface CreateOptions extends SaveOptions {
ordered?: boolean;
aggregateErrors?: boolean;
}

interface RemoveOptions extends SessionOption, Omit<mongodb.DeleteOptions, 'session'> {}

const Model: Model<any>;
Expand Down Expand Up @@ -217,7 +221,8 @@ declare module 'mongoose' {
>;

/** Creates a new document or documents */
create<DocContents = AnyKeys<TRawDocType>>(docs: Array<TRawDocType | DocContents>, options?: SaveOptions): Promise<THydratedDocumentType[]>;
create<DocContents = AnyKeys<TRawDocType>>(docs: Array<TRawDocType | DocContents>, options: CreateOptions & { aggregateErrors: true }): Promise<(THydratedDocumentType | Error)[]>;
create<DocContents = AnyKeys<TRawDocType>>(docs: Array<TRawDocType | DocContents>, options?: CreateOptions): Promise<THydratedDocumentType[]>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add TypeScript types to indicate that, if immediateError: false, then the return type is Promise<Array<THydratedDocumentType | Error>>

create<DocContents = AnyKeys<TRawDocType>>(doc: DocContents | TRawDocType): Promise<THydratedDocumentType>;
create<DocContents = AnyKeys<TRawDocType>>(...docs: Array<TRawDocType | DocContents>): Promise<THydratedDocumentType[]>;

Expand Down