talha-asad / mongoose-url-slugs

Create URL compatiable slugs on mongoose models, ensuring uniqueness.
MIT License
40 stars 22 forks source link

Error thrown if source field doesn't exist #6

Closed Soviut closed 7 years ago

Soviut commented 9 years ago

I was doing some unit testing and intentionally feeding my model invalid and missing data. Rather than getting a 400 error for malformed syntax as expected, mongoose-url-slugs threw a type error because the field it was using to generate the slug didn't exist.

Uncaught TypeError: Cannot call method 'toLowerCase' of undefined

This seems to be due to the pre('validate') function not checking for the existence of field before running the generator. https://github.com/mindblaze/mongoose-url-slugs/blob/master/index.js#L104

The existence of the field should be checked and an error returned on next() if the field is missing.

Soviut commented 9 years ago

I'm playing around with a fork to see if I can get this working but so far no luck. The test I'm using is:

  it('should not slugify fields that dont exist', function(done) {
    TestObj.create({}, function(err, obj) {
      TestObj.create({}, function(err, obj) {
        done();
      });
    });
  });

I don't have any expectations in it yet because I'm not actually sure what the behaviour should be. I'm currently having it generate "undefined" but it feels like it shouldn't add the field at all if there's an error.

talha-asad commented 9 years ago

Can you please provide a sample for this? I think its important to decide what should happen in such a case. I am not sure what you mean when you say invalid or missing data.

Soviut commented 9 years ago

I mean I was feeding my tests objects that were missing the field which mongoose-source-urls targets to generate the slug from. Without the target field in the object, it has nothing to slugify, which makes sense, but the error it raises isn't in keeping with what mongoose would raise for a similar issue.

viztastic commented 9 years ago

@talha-asad Here's a real example... My model looks like this:


var mongoose = require('mongoose'),
    Schema = mongoose.Schema,
    urlSlug = require('mongoose-url-slugs');

var "contentSchema" = new Schema({
     "name" : { "value" : {type : String, required: true} }
     "description" : String
});

contentSchema.plugin(urlSlug('name.value'));

I was attempting to create a slug based on the 'name.value' field (I need to have value nested inside of name for localisation reasons later on).

When i did this, the following error was thrown

''' D:\dev\salis\node_modules\mongoose-url-slugs\index.js:4 var slug = text.toLowerCase().replace(/([^a-z0-9\-\_]+)/g, separator).replac ^ TypeError: Cannot read property 'toLowerCase' of undefined at Object.defaultURLSlugGeneration [as generator] (D:\dev\salis\node_modules\mongoose-url-slugs\index.js:4:18) at model.<anonymous> (D:\dev\salis\node_modules\mongoose-url-slugs\index.js:104:29) at model._next (D:\dev\salis\node_modules\mongoose\node_modules\hooks-fixed\hooks.js:62:30) at model.proto.(anonymous function) [as $__original_validate] (D:\dev\salis\node_modules\mongoose\node_modules\hooks-fixed\hooks.js:108:20) at model.wrappedPointCut [as validate] (D:\dev\salis\node_modules\mongoose\lib\document.js:1573:21) at model.Object.defineProperty.value.fn (D:\dev\salis\node_modules\mongoose\lib\schema.js:188:16) at model._next (D:\dev\salis\node_modules\mongoose\node_modules\hooks-fixed\hooks.js:62:30) at model.proto.(anonymous function) [as $__original_save] (D:\dev\salis\node_modules\mongoose\node_modules\hooks-fixed\hooks.js:108:20) at model.wrappedPointCut [as save] (D:\dev\salis\node_modules\mongoose\lib\document.js:1573:21) at D:\dev\salis\node_modules\mongoose\lib\model.js:1607:35 at D:\dev\salis\node_modules\mongoose\node_modules\async\lib\async.js:570:21 at D:\dev\salis\node_modules\mongoose\node_modules\async\lib\async.js:249:17 at D:\dev\salis\node_modules\mongoose\node_modules\async\lib\async.js:125:13 at Array.forEach (native) at _each (D:\dev\salis\node_modules\mongoose\node_modules\async\lib\async.js:46:24) at async.each (D:\dev\salis\node_modules\mongoose\node_modules\async\lib\async.js:124:9) '''

My recommendation is that:

talha-asad commented 8 years ago

Sorry it took me a while to get back to this, i am planning to fix this in the next version.

tommhuth commented 8 years ago

how is the bug fix for this coming along? This seems like an easy thing to fix, and the whole point of validation is to guard against things like this, so there can be no assumptions about what comes in.

talha-asad commented 8 years ago

@tommhuth You're right this is a pretty easy thing to fix, I just haven't been able to get the time to fix it. I am happy to merge a PR if someone is willing to put in the effort.

talha-asad commented 7 years ago

New implementation, starting from V1

Slug depends on single field, which is not defined uses 'undefined'

Slug depends on multiple fields if one of them is not defined, it is skipped if all of them are undefined, uses 'undefined'

With indexSparse set to true slug field is not set

undefinedVal is a configurable option, defaults to 'undefined'