fix: add validation support for Union schema types

[codervipul775]

This PR implements validation for Union schema types to properly validate required fields and custom validators in union schemas and their array elements.

Breaking Changes: None

Test Results: All 3946 tests passing with 5 new Union validation tests

This commit message follows best practices with:

Clear prefix (fix:) indicating the type of change
Concise subject line summarizing the main change
Detailed body explaining what was changed and why
List of specific files and modifications
Reference to the issue being fixed
Test coverage information

[Copilot]

Pull Request Overview

This PR adds validation support for Union schema types in Mongoose, enabling proper validation of required fields and custom validators within union schemas and their array elements.

• Implements doValidate and doValidateSync methods for the Union SchemaType to validate values against union member schemas
• Adds validation path optimization checks to prevent Union types from being skipped during validation
• Includes best match scoring logic to select the most appropriate schema when casting union values

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
lib/schema/union.js	Implements validation methods (doValidate, doValidateSync) and enhances casting logic with best match scoring for object values
lib/document.js	Adds instance !== 'Union' checks to validation path optimization to ensure Union types are not skipped during validation
test/schema.union.validation.test.js	Comprehensive test coverage for Union validation including required fields, custom validators, array validation, and synchronous validation

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] Extra blank line should be removed for consistency with the codebase style.

Suggested change

[timheerwagen]

It makes no sense to perform an additional test to see if the same incorrect input fails with an extra field. Instead, it needs to check that the arbitrary field has been removed from the object. It is better to not error out on creation and check the saved document to see if the arbitrary field was removed.

[codervipul775]

Thanks @timheerwagen for the review. I have made the necessary changes. Please have a look into it

[vkarpov15]

I agree that right now unions of objects are a bit tough to work with, and this general idea represents an improvement.

However, we need a way to tie which schematypes, if any, pass casting before running validation. This PR completely decouples the casting logic and validation logic, so if a value casts as one member of the union and validates as another member of the union then it goes through. For example, the following case succeeds because 12 casts as a number and then passes the string's validation (no validation):

const mongoose = require('mongoose');

const { inspect } = require('util');

const TestSchema = new mongoose.Schema({
  product: {
    type: mongoose.Schema.Types.Union,
    of: [{ type: Number, required: true, min: 44 }, String],
  },
});

const TestModel = mongoose.model("test", TestSchema);

const main = async () => {
  await mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');

  const data = await TestModel.create({
    product: 12,
  });

  console.log(inspect(data, { depth: Infinity }));
  
  process.exit();
};

main();

[codervipul775]

@vkarpov15 Thank you for the detailed feedback! You're absolutely right about the coupling issue between casting and validation.

I've updated the implementation to tie casting and validation together:

Changes made:

Added tracking mechanism: Introduced a castSchemaTypeSymbol to store which schema type was used during casting

Modified cast() and applySetters(): Now store the schema type that successfully cast the value on subdocuments

Updated doValidate() and doValidateSync():

First check if a schema type was stored during casting and use that for validation
For primitives (non-subdocs), re-cast to determine which schema type should validate
Fall back to trying all schema types only if no match is found
Test added: Created a test case using your exact example with of: [{ type: Number, required: true, min: 44 }, String] that verifies:

Value 12 casts as Number and fails validation (12 < 44)
Value 50 casts as Number and passes validation (50 > 44)
String values cast and validate correctly as String
All 3947 tests pass with the new implementation.

This ensures that whichever schema type successfully casts a value is also the one that validates it, preventing the validation bypass issue you identified.

Please review it.

[vkarpov15]

Overall I'm not convinced this PR is heading in the right direction, there are a lot of fundamental gaps here. With the way unions are currently implemented I don't think supporting unions of subdocuments is practical, embedded discriminators are a better choice for that use case.

I'd rather fogure out a way to better support validation for unions of 2 primitive types, or union of primitive and object. The attempts to reconcile different object types seems to be unrelated to the issue of using the correct union schematype's validations.

[vkarpov15]

Going for a best match scoring is a bit overkill IMO, this sort of pattern will be difficult to explain and document. I would prefer to keep more of a "first entry in the union that matches" approach.

[vkarpov15]

Given the complexity of objects in union types, I think we might just recommend using embedded discriminators instead.

[vkarpov15]

The problem is that the value was already casted, so we're not casting the original value here.

Because we're not getting the original value passed in to cast(), we also can't replicate the "check if casting returns the same value, and if so use that value" behavior here.

[codervipul775]

Hey @vkarpov15

Thank you for the detailed feedback! I've implemented all the requested changes:

Changes Made:

Removed best-match scoring - Eliminated bestMatch, bestMatchScore, and preservedFields logic entirely. Now using simple "first match wins" strategy as suggested.

Fixed re-casting issue - Validation now uses a hybrid approach:

For subdocuments: Uses the stored castSchemaTypeSymbol to validate against the same schema type that was used during casting (no re-casting)
For primitives: Re-casts during validation to determine which schema type accepts the value (necessary since primitives can't store metadata)
Simplified to focus on basic use cases - Updated all tests from complex SubSchema1 | SubSchema2 scenarios to simple cases:

Number | SubSchema (primitive + object)
String | SubSchema (primitive + object)
Primitives with custom validators
Test Results:

All 6 Union validation tests passing
Total test suite: 3947 passing, 49 pending
The implementation now aligns with your guidance to keep Union simple and recommend embedded discriminators for complex subdocument scenarios.

Let me know if there's anything else you'd like me to adjust!

[vkarpov15]

The test suite is great and there's some good work in this PR, but we should try an approach which stores the schema type for a given union path in the parent document's $__, e.g. looking at this.$__.unionTypeForPath to get the schema type associated with a specific union path, instead of re-casting when running validation. We just need to make sure that works correctly with arrays of unions and maps of unions.

[vkarpov15]

I really don't like re-casting as part of validation. For one thing, while casting is almost always going to be deterministic, there's no guarantee that casting the casted value will give you the same result back. This also breaks the convention that casting and validation are separate.

[vkarpov15]

I also don't like relying on $__[castSchemaTypeSymbol] here because we need to make sure we clear castSchemaTypeSymbol when setting to another document, e.g. doc1.value = doc2.value should override doc2's castSchemaTypeSymbol. This kind of dangling state tends to cause bugs.

[vkarpov15]

I presume this is so Mongoose doesn't skip unions when getting paths to validate? If so, I'd prefer to add an explicit exception to avoid unions instead of adding a fake validator.