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

[Types] DocumentArray.caster should be Types.Subdocument, not typeof Types.Subdocument #15179

Open
2 tasks done
JstnMcBrd opened this issue Jan 13, 2025 · 4 comments
Open
2 tasks done
Labels
backwards-breaking typescript Types or Types-test related issue / Pull Request

Comments

@JstnMcBrd
Copy link
Contributor

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

8.9.3

Node.js version

20.15.1

MongoDB server version

8

Typescript version (if applicable)

5.6.3

Description

In Schema.Types.Array, the caster is an instance of SchemaType. It would make sense that for other SchemaTypes, the caster would be an instance of a subclass of SchemaType.

However, in DocumentArray the caster is typeof Types.Subdocument, which is a function, not an instance of SchemaType itself. Instead, caster should just be Types.Subdocument.

https://github.com/Automattic/mongoose/blob/master/types/schematypes.d.ts#L428

     class DocumentArray extends SchemaType implements AcceptsDiscriminator {
        /** This schema type's name, to defend against minifiers that mangle function names. */
        static schemaName: 'DocumentArray';

        static options: { castNonArrays: boolean; };

        discriminator<D>(name: string | number, schema: Schema, value?: string): Model<D>;
        discriminator<T, U>(name: string | number, schema: Schema<T, U>, value?: string): U;

        /** The schema used for documents in this array */
        schema: Schema;

        /** The constructor used for subdocuments in this array */
-        caster?: typeof Types.Subdocument;
+        caster?: Types.Subdocument;

        /** Default options for this SchemaType */
        defaultOptions: Record<string, any>;
      }

Steps to Reproduce

Here's an example of how the typing causes issues:

function isStringArray(schemaType: Schema.Types.Array | Schema.Types.DocumentArray): boolean {
  return isStringType(schemaType.caster);
  // SchemaType | typeof Types.Subdocument | undefined is not assignable to SchemaType | undefined
}

function isStringType(schemaType?: SchemaType): boolean {
  if (!schemaType) return false;
  return schemaType instanceof Schema.Types.String;
}

Expected Behavior

No response

@vkarpov15 vkarpov15 added this to the 8.9.6 milestone Jan 14, 2025
@vkarpov15 vkarpov15 added the typescript Types or Types-test related issue / Pull Request label Jan 14, 2025
@vkarpov15
Copy link
Collaborator

It looks like the typings are correct, consider the following script:

'use strict';

const mongoose = require('mongoose');
const { Schema } = mongoose;

const schema = new Schema({ docArray: [new Schema({ name: String })] });
console.log(schema.path('docArray').caster);

Output is:

$ node gh-15179.js 
[Function: EmbeddedDocument] {
  schema: Schema {
    obj: { name: [Function: String] },
    paths: { name: [SchemaString], _id: [SchemaObjectId] },
    aliases: {},
    subpaths: {},
    virtuals: { id: [VirtualType] },
    singleNestedPaths: {},
    nested: {},
    inherits: {},
    callQueue: [],
    _indexes: [],
    _searchIndexes: [],
    methods: {},
    methodOptions: {},
    statics: {},

So yes, caster is inconsistent between Array and DocumentArray: it is a function for DocumentArray, and a SchemaType instance for Array.

Consider using getEmbeddedSchemaType() instead if you want to check if a given SchemaType is an array of strings.

@vkarpov15 vkarpov15 removed this from the 8.9.6 milestone Jan 15, 2025
@vkarpov15 vkarpov15 added the help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary label Jan 15, 2025
@JstnMcBrd
Copy link
Contributor Author

The array of strings example is contrived.

Also, I don't think that console output demonstrates that caster is a function. Consider the following playground example:

await mongoose.connect('mongodb://localhost:27017/mongoose_test');
const schema = new mongoose.Schema({
	docArray: {
    	type: [
        	new mongoose.Schema({ test: String })
        ]
    }
});
const docArray = schema.path('docArray');
console.log('is DocumentArray', docArray instanceof mongoose.Schema.Types.DocumentArray);
console.log('is Array', docArray instanceof mongoose.Schema.Types.Array); // DocumentArrays are instances of Array too
console.log(docArray.caster.schema); // If `caster` is a function, this should not be possible

That gives the following output:

'is DocumentArray' true
'is Array' true
{ obj: { test: [Function: String] },
  paths: 
   { test: ...,
     _id: ... },
  aliases: {},
  subpaths: {},
  virtuals: ...,
  singleNestedPaths: {},
  nested: {},
  inherits: {},
  callQueue: [],
  _indexes: [],
  methods: {},
  methodOptions: {},
  statics: {},
  tree: ...,
  query: {},
  childSchemas: [],
  plugins: [],
  '$id': 6,
  mapPaths: [],
  s: ...,
  _userProvidedOptions: {},
  options: ... }

If caster was actually a function, you would not be able to access members like a true class instantiation. Currently, the types will not allow this:

if (docArray instanceof Schema.Types.DocumentArray) {
  console.log(schemaType.caster.schema);
  // ERROR: Property 'schema' does not exist on type 'typeof Subdocument'.ts(2339)
}

Also, this example appears to show that DocumentArray is a subclass of Array. Should not their caster fields be compatible?

@vkarpov15
Copy link
Collaborator

"If caster was actually a function, you would not be able to access members like a true class instantiation." <-- this doesn't seem to be true. Functions are objects in JavaScript, so you can assign properties to functions: const fn = () => {}; fn.myProp = 42; console.log(fn.myProp);

"Also, this example appears to show that DocumentArray is a subclass of Array. Should not their caster fields be compatible?" DocumentArray is a subclass of Array, that is true. We will consider changing this in our next major release. In JavaScript, there's no such restriction that DocumentArray's caster needs to be in any way compatible with Array's, but we also need to be compatible with TypeScript.

Copy link

github-actions bot commented Feb 1, 2025

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Feb 1, 2025
@vkarpov15 vkarpov15 removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary Stale labels Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-breaking typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests

2 participants