Fix: in some Number casts there were an assert twice and was not handling undefined

[p3x-robot]

fixes: #8711

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. The two fields below are mandatory.

If you're making a change to documentation, do not modify a .html file directly. Instead find the corresponding .pug file or test case in the test/docs directory.

Summary

#8711

Examples

[2020/03/23 10:15:41.958] [WORKER 002] [PID: 002284]  [ERROR] MongooseError [CastError]: Cast to number failed for value "undefined" at path "year" for model "Ticket"
    at new CastError (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/error/cast.js:39:11)
    at SchemaNumber.cast (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schema/number.js:384:11)
    at SchemaNumber.handleSingle (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schema/number.js:393:15)
    at SchemaNumber.castForQuery (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schema/number.js:434:20)
    at SchemaNumber.SchemaType.castForQueryWrapper (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schematype.js:1388:17)
    at cast (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/cast.js:296:39)
    at model.Query.Query.cast (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/query.js:4685:12)
    at model.Query.Query._castConditions (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/query.js:1862:10)
    at model.Query.<anonymous> (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/query.js:1889:8)
    at model.Query._wrappedThunk [as _find] (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/helpers/query/wrapThunk.js:16:8)
    at /home/adony/adony.ngerp.sygnus.hu/node_modules/kareem/index.js:369:33
    at processTicksAndRejections (internal/process/task_queues.js:79:11) {
  stringValue: '"undefined"',
  kind: 'number',
  value: undefined,
  path: 'year',
  reason: AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

    assert.ok(!isNaN(val))

      at castNumber (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/cast/number.js:17:10)
      at SchemaNumber.cast (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schema/number.js:382:12)
      at SchemaNumber.handleSingle (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schema/number.js:393:15)
      at SchemaNumber.castForQuery (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schema/number.js:434:20)
      at SchemaNumber.SchemaType.castForQueryWrapper (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/schematype.js:1388:17)
      at cast (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/cast.js:296:39)
      at model.Query.Query.cast (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/query.js:4685:12)
      at model.Query.Query._castConditions (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/query.js:1862:10)
      at model.Query.<anonymous> (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/query.js:1889:8)
      at model.Query._wrappedThunk [as _find] (/home/adony/adony.ngerp.sygnus.hu/node_modules/mongoose/lib/helpers/query/wrapThunk.js:16:8)
      at /home/adony/adony.ngerp.sygnus.hu/node_modules/kareem/index.js:369:33
      at processTicksAndRejections (internal/process/task_queues.js:79:11) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: false,
    expected: true,
    operator: '=='
  },
  message: 'Cast to number failed for value "undefined" at path "year" for model "Ticket"',
  name: 'CastError',
  model: Model { Ticket }
}

[vkarpov15]

Based on your stack trace, there is only 1 error, just that error has a reason property that is itself an error.

Also, your PR doesn't do anything. Using double equals to check for null is a common shorthand because v == null is equivalent to v === null || v === undefined.

[AbdelrahmanHafez]

@vkarpov15 notice that the PR also removes the isNaN check assert.ok(!isNaN(val);

[p3x-robot]

@vkarpov15 notice that the PR also removes the isNaN check assert.ok(!isNaN(val);

yes, because there is already the same assert:

mongoose/lib/cast/number.js

Line 29 in bfc10f5

assert.ok(!isNaN(val));

why are there the same code twice? weird.

[p3x-robot]

weird, why mongoose not working with Number as undefined that's all I am saying. Super weird. COVID-19!!! :)

[p3x-robot]

Now it will be like this (the assert is there, but allow for working with undefined).

'use strict';

const assert = require('assert');

/*!
 * Given a value, cast it to a number, or throw a `CastError` if the value
 * cannot be casted. `null` and `undefined` are considered valid.
 *
 * @param {Any} value
 * @param {String} [path] optional the path to set on the CastError
 * @return {Boolean|null|undefined}
 * @throws {Error} if `value` is not one of the allowed values
 * @api private
 */

module.exports = function castNumber(val) {

  if (val == null) {
    return val;
  }
  if (val === '') {
    return null;
  }

  if (typeof val === 'string' || typeof val === 'boolean') {
    val = Number(val);
  }

  assert.ok(!isNaN(val));
  if (val instanceof Number) {
    return val.valueOf();
  }
  if (typeof val === 'number') {
    return val;
  }
  if (!Array.isArray(val) && typeof val.valueOf === 'function') {
    return Number(val.valueOf());
  }
  if (val.toString && !Array.isArray(val) && val.toString() == Number(val)) {
    return Number(val);
  }

  assert.ok(false);
};

[p3x-robot]

@vkarpov15 notice that the PR also removes the isNaN check assert.ok(!isNaN(val);

if you keep this dual assert you will like this code:

how can i fix this error as it says, it to not throw an error with undefined Number Mongoose Model?

[p3x-robot]

Mongoose doc for Number cast says:

Given a value, cast it to a number, or throw a `CastError` if the value
 * cannot be casted. `null` and `undefined` are considered valid.

But if this cast happening as castNumber(undefined), because the assert on the top will throw an Error.

[p3x-robot]

Thanks for helping!

[vkarpov15]

Works for me 👍

[AbdelrahmanHafez]

This is a breaking change, we already have tests asserting that null, and undefined are not allowed for SchemaType of number.

@vkarpov15 should I remove the now-failing tests that assert that?

 1) types array                                                                                 
       of number                                                                                 
         allows nulls:                                                                           
     Uncaught AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:         
                                                                                                 
  assert.ok(err)                                                                                 
                                                                                                 
      at mongoose\test\types.array.test.js:1783:18      
      at mongoose\lib\model.js:4837:16                  
      at mongoose\lib\helpers\promiseOrCallback.js:24:16
      at mongoose\lib\model.js:4860:21                  
      at model.<anonymous> (lib\model.js:491:7)                                                  
      at mongoose\node_modules\kareem\index.js:315:21   
      at next (node_modules\kareem\index.js:209:27)                                              
      at mongoose\node_modules\kareem\index.js:182:9    
      at mongoose\node_modules\kareem\index.js:507:38   
      at processTicksAndRejections (internal/process/task_queues.js:79:11)                       
                                                                                                 
  2) types.number                                                                                
       undefined throws number cast error:                                                       
                                                                                                 
      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:                      
                                                                                                 
true !== false                                                                                   
                                                                                                 
      + expected - actual                                                                        
                                                                                                 
      -true                                                                                      
      +false                                                                                     
                                                                                                 
      at Context.<anonymous> (test\types.number.test.js:38:12)                                   
      at processImmediate (internal/timers.js:456:21)                                            

[AbdelrahmanHafez]

We also shouldn't be allowing NaN to go through if we decide that we want to allow undefined.

This may do the trick:

assert.ok(!isNaN(val) || val === undefined);

[vkarpov15]

@AbdelrahmanHafez nope, apparently the fact that Mongoose allows null in number arrays but not undefined even if required is not set is a very old quirk that goes all the way back to 2012 with this commit: 1298fe9 . Since we test for this quirk, we need to support it, but just move it up to the array casting layer rather than the number casting layer.

I will remove the test that asserts SchemaNumber#cast() throws on undefined though. That's a more internal test. This quirk is to fix issue #840, which only impacted arrays of numbers.

[AbdelrahmanHafez]

@vkarpov15 Great. Perhaps we should be dropping the support for that quirk in the 6.0.