🐛fix: improve inferred Schema Type #12007

iammola · 2022-06-29T00:16:21Z

Fixes #12002
Fixes #12011
Fixed #12016

Summary

It uses the correct Type.ObjectId when inferring ObjectId fields (fixes the issue) and correctly infers the type for sub-documents defined with the typeKey and all sub-document arrays. Previously, the ResolvePathType helper type didn't match any Schema types in its pyramid of type checks. The only array check looped the same ResolvePathType, so it never matched any Schema in arrays and it goes deeper in arrays for deeper nested Schemas.

I'm not sure how the other change will be received, but currently, any array that isn't explicitly marked with required: true gets resolved to be optional. I understand why, but the default value for an array schema type is an empty one unless the user explicitly sets it to undefined.

So I think it'll be better if array paths that don't have their defaults set to undefined aren't marked as optional types. Mongoose already allows document fields to be skipped when creating them with a method like Model.create. So if in the result of the find* queries, the arrays are marked as possibly undefined. It could cause unnecessary errors with TS.

Example

// Given Schema
const Author = new Schema({
  contractedTo: { type: Schema.Types.ObjectId, required: true },
  tags: [{ type: String }],
  contact: {
    mail: String,
    phone: String
  },
  profile: {
    type: new Schema({
      age: Number,
      username: { type: String, required: true }
    }),
    required: true
  },
  booksReleasedPerYear: {
    type: [new Schema({
      year: { type: Number, required: true },
      count: { type: Number, required: true }
    })]
  }
})

// It should infer this type
type AuthorSchema = {
  contractedTo: Types.ObjectId;
  tags?: string[] | undefined;
  contact?: {
    mail?: string | undefined;
    phone?: string | undefined;
  } | undefined,
  profile: {
    age?: number | undefined;
    username: string;
  };
  booksReleasedPerYear?: Array<{
    year: number;
    count: number;
  }>
}

- Correctly detect types for Schema[] and Schema in typeKey - Use correct ObjectId type Fixes #12002

The default value for an array field is an empty array unless specifed to be undefined. This checks if the path explicitly sets the default property to `undefined`

For bb2ef25

imranbarbhuiya · 2022-06-29T18:48:39Z

Does this pr fixes #12016 too?

iammola · 2022-06-29T20:42:57Z

Does this pr fixes #12016 too?

@imranbarbhuiya... I didn't get the issue in the 6.4.x and it's the same in this PR as well. I don't know how he's getting the the optional properties in the type.

…own`

iammola · 2022-06-29T21:27:33Z

To the maintainer helping me run the workflows,

If the Schema type itself was hovered in an editor like VS Code, you'll see the types are set exactly as they should be, but it was the result of the InferredSchemaType that set them to unknown for some reason. I traced it down to the cause of the problem, and the test was failing because the Schema.Types.Mixed matched this check,

mongoose/types/inferschematype.d.ts

Line 153 in 79c9ce4

    
           PathValueType extends { [K: string]: SchemaDefinitionProperty } ? ObtainDocumentType<PathValueType, any, TypeKey> :

. The {} and ObjectConstructor types also caused issues, I was able to solve them but ultimately decided to remove their checks as well and let those three types resolve to unknown

It's meant to match nested paths that aren't Schema (I made that change in Automattic/mongoose@2f1d2d9 (#12007)).

I don't know how you feel about that, but since unknown matches all other types, I hope it can be used because I wasn't able to find a way to check for those nested paths and for those three types as well.

test/types/schema.test.ts

types/inferschematype.d.ts

This commit reverts 4037696

mohammad0-0ahmad · 2022-06-30T16:15:35Z

I've created PR in your fork repo, check the changes and merge it if that will solve all mentioned issues.

mohammad0-0ahmad · 2022-06-30T23:16:14Z

Does this pr fixes #12016 too?

@iammola would you like to cover this issue in this PR ?
I think you can cover it by refactoring IsPathRequired helper

mohammad0-0ahmad · 2022-06-30T23:36:38Z

Does this pr fixes #12016 too?

@iammola would you like to cover this issue in this PR ? I think you can cover it by refactoring IsPathRequired helper

@iammola If you include it in this PR, please don't forget to mention the issue in the description. 🥇

iammola · 2022-06-30T23:43:41Z

Yeah @mohammad0-0ahmad. I'm on it.

I've been thinking what would be allowed? Should the default type match the typeKey's type or any valid SchemaDefinitionProperty (StringConstructor, NumberConstructor, ...) should make the path not undefined?

Because if the default and typeKey's types are different, does that mean it's a mixed path?

But in the case of Dates where you may default it to Date.now(). Then the default's type is a number while the typeKey's type is a Date

mohammad0-0ahmad · 2022-06-30T23:52:52Z

Yeah @mohammad0-0ahmad. I'm on it.

I've been thinking what would be allowed? Should the default type match the typeKey's type or any valid SchemaDefinitionProperty (StringConstructor, NumberConstructor, ...) should make the path not undefined?

Because if the default and typeKey's types are different, does that mean it's a mixed path?

But in the case of Dates where you may default it to Date.now(). Then the default's type is a number while the typeKey's type is a Date

I think we can make it match the TypeKey's type for now.
We can ask @Uzlopak @vkarpov15 @AbdelrahmanHafez.

iammola · 2022-07-01T00:31:06Z

types/inferschematype.d.ts

@@ -75,7 +75,7 @@ type IsPathRequired<P, TypeKey extends TypeKeyBaseType> =
      ? P extends { default: undefined }
        ? false
        : true
-    : P extends PathWithTypePropertyBaseType<TypeKey>
+    : P extends (Record<TypeKey, NumberSchemaDefinition | StringSchemaDefinition | BooleanSchemaDefinition | DateSchemaDefinition>)


I assumed not all of typeKey's types should be allowed to be remove the optional flag on that path.

So I set it to these 4. Because I think they're the only ones that should be able to provide defaults.

So if you don't want the types to be limited, I'll revert the whole commit. @mohammad0-0ahmad

iammola · 2022-07-01T00:33:27Z

types/inferschematype.d.ts

-          PathValueType extends DateConstructor | 'date' | 'Date' | typeof Schema.Types.Date ? Date :
+      PathValueType extends StringSchemaDefinition ? PathEnumOrString<Options['enum']> :
+        PathValueType extends NumberSchemaDefinition ? number :
+          PathValueType extends DateSchemaDefinition ? Date :


These changes are just because I noticed they're defined in their respective SchemaDefinition types.

mohammad0-0ahmad · 2022-07-01T00:45:28Z

Sorry @iammola, I can't contributing for now, I will check changes when I can as soon as possible, please consider checking TS benchmark while refactoring types to avoid slower performance.

vkarpov15

This is great, thanks!

mohammad0-0ahmad · 2022-07-01T23:30:32Z

@iammola sorry for being late, But I would like to say well done : )

iammola · 2022-07-01T23:44:15Z

@iammola sorry for being late, But I would like to say well done : )

Thank you, man... I really appreciate it. I honestly didn't do much, but I'll accept it 🙏

iammola added 6 commits June 28, 2022 22:05

🏷️types: fix types

bb2ef25

- Correctly detect types for Schema[] and Schema in typeKey - Use correct ObjectId type Fixes #12002

🏷️types: improve inferred types for arrays

2e51745

The default value for an array field is an empty array unless specifed to be undefined. This checks if the path explicitly sets the default property to `undefined`

✅test: add tests

73706cb

For bb2ef25

🏷️types: correct friendId type

6277c4f

🐛fix: handle deeper schema arrays

687d4db

🐛fix: corrrectly filter default: undefined aray

e3679a8

iammola changed the title ~~🐛fix: improved inferred Schema Type~~ 🐛fix: improve inferred Schema Type Jun 29, 2022

iammola added 2 commits June 29, 2022 13:02

🐛fix: resolves type of nested paths that isn't a schema

79c9ce4

🚨lint: fix indentation

7d280a2

iammola added 2 commits June 29, 2022 21:39

🐛fix: use correct type in test schema

2f1d2d9

✅test: remove mixed schema type test

5ce27ac

🏷️types: allow Mixed, {} and ObjectConstructor resolve to `unkn…

d31aa08

…own`

Uzlopak reviewed Jun 29, 2022

View reviewed changes

test/types/schema.test.ts Show resolved Hide resolved

🏷️types: use unknown as expected mixed schema type

4123249

mohammad0-0ahmad suggested changes Jun 30, 2022

View reviewed changes

test/types/schema.test.ts Show resolved Hide resolved

test/types/schema.test.ts Show resolved Hide resolved

test/types/schema.test.ts Outdated Show resolved Hide resolved

test/types/schema.test.ts Show resolved Hide resolved

types/inferschematype.d.ts Show resolved Hide resolved

mohammad0-0ahmad reviewed Jun 30, 2022

View reviewed changes

types/inferschematype.d.ts Show resolved Hide resolved

iammola and others added 5 commits June 30, 2022 08:46

🐛fix: reviewed changes

d12af93

Restore schema.test.ts

4037696

Revert "Restore schema.test.ts"

3def8c3

This commit reverts 4037696

🚨lint: fix warnings

27614de

Fix TS errors And refactor inferschematype.d.ts file

1abdb34

mohammad0-0ahmad and others added 2 commits July 1, 2022 00:34

Coverage for array path implicit type.

1c82c05

Merge pull request #1 from mohammad0-0ahmad-forks/_12007

c64f97b

mohammad0-0ahmad approved these changes Jun 30, 2022

View reviewed changes

iammola added 3 commits July 1, 2022 01:23

🐛 fix: make paths with default specified required

814a0db

🐛 fix: limit allowed types to be required with default

dc5e52a

✅ test: add test for schema paths with default

8aa7f53

iammola commented Jul 1, 2022

View reviewed changes

🚨 lint: fix errors

ca6f97b

vkarpov15 approved these changes Jul 1, 2022

View reviewed changes

vkarpov15 added this to the 6.4.2 milestone Jul 1, 2022

vkarpov15 merged commit 87a6edc into Automattic:master Jul 1, 2022

iammola deleted the inferschematype branch July 2, 2022 04:12

vkarpov15 mentioned this pull request Jul 2, 2022

[help] InferSchemaType, accessing _id field #11967

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛fix: improve inferred Schema Type #12007

🐛fix: improve inferred Schema Type #12007

iammola commented Jun 29, 2022 •

edited

imranbarbhuiya commented Jun 29, 2022 •

edited

iammola commented Jun 29, 2022

iammola commented Jun 29, 2022

mohammad0-0ahmad commented Jun 30, 2022

mohammad0-0ahmad commented Jun 30, 2022 •

edited

mohammad0-0ahmad commented Jun 30, 2022 •

edited

iammola commented Jun 30, 2022

mohammad0-0ahmad commented Jun 30, 2022 •

edited

iammola Jul 1, 2022

iammola Jul 1, 2022 •

edited

iammola Jul 1, 2022

mohammad0-0ahmad commented Jul 1, 2022 •

edited

vkarpov15 left a comment

mohammad0-0ahmad commented Jul 1, 2022

iammola commented Jul 1, 2022

🐛fix: improve inferred Schema Type #12007

🐛fix: improve inferred Schema Type #12007

Conversation

iammola commented Jun 29, 2022 • edited

Summary

Example

imranbarbhuiya commented Jun 29, 2022 • edited

iammola commented Jun 29, 2022

iammola commented Jun 29, 2022

mohammad0-0ahmad commented Jun 30, 2022

mohammad0-0ahmad commented Jun 30, 2022 • edited

mohammad0-0ahmad commented Jun 30, 2022 • edited

iammola commented Jun 30, 2022

mohammad0-0ahmad commented Jun 30, 2022 • edited

iammola Jul 1, 2022

Choose a reason for hiding this comment

iammola Jul 1, 2022 • edited

Choose a reason for hiding this comment

iammola Jul 1, 2022

Choose a reason for hiding this comment

mohammad0-0ahmad commented Jul 1, 2022 • edited

vkarpov15 left a comment

Choose a reason for hiding this comment

mohammad0-0ahmad commented Jul 1, 2022

iammola commented Jul 1, 2022

iammola commented Jun 29, 2022 •

edited

imranbarbhuiya commented Jun 29, 2022 •

edited

mohammad0-0ahmad commented Jun 30, 2022 •

edited

mohammad0-0ahmad commented Jun 30, 2022 •

edited

mohammad0-0ahmad commented Jun 30, 2022 •

edited

iammola Jul 1, 2022 •

edited

mohammad0-0ahmad commented Jul 1, 2022 •

edited