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

Correctly infer types on document arrays #12884

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

JavaScriptBach
Copy link
Contributor

@JavaScriptBach JavaScriptBach commented Jan 7, 2023

Summary

Fix #12882.

The key insight here is that arrays of scalars and arrays of objects have different type inference logic. For example:

  1. type: [{type: String, required: true}] is an array of strings.
  2. type: [{type: "String", required: true}] is also an array of strings.
  3. type: [{type: {foo: {type: String}, required: true}] is an array of objects that contains a field named type. Note here that type must be interpreted as a real key, not a type key.

So if we have an array, we need to check if it's an array of scalars or objects. This is done as follows:

  1. If the object has a type key that isn't callable and isn't a string, it's an object, so we need to call ObtainDocumentType to make sure that its keys are interpreted correctly. (i.e. if the object has a key called type, this is a real key, not a type key)
  2. In all other cases, we continue to assume that the array is a scalar.

Examples

Adds tests.

@JavaScriptBach JavaScriptBach marked this pull request as ready for review January 7, 2023 01:01
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor question/suggestion.

// If Item is a schema, infer its type.
Types.DocumentArray<InferSchemaType<Item>> :
Item extends Record<TypeKey, any>?
Item[TypeKey] extends Function ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it also needs to handle the case where Item[TypeKey] is a string. type: 'String' is equivalent to type: String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I handled this case and added tests

@vkarpov15 vkarpov15 added this to the 6.8.4 milestone Jan 17, 2023
@vkarpov15 vkarpov15 merged commit ed8eacf into Automattic:master Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InferSchemaType: array of subdocuments with a "type" key is not correctly inferred
2 participants