search

olivernn / lunr.js

A bit like Solr, but much smaller and not as bright
http://lunrjs.com
MIT License
8.96k stars 548 forks source link

Bug removing a non-existent document #22

Closed piranna closed 11 years ago

piranna commented 11 years ago

When trying to remove a document from the index that the ID doesn't exists (maybe because an automatic clean-up) it raise an error because this case is not check:

lunr.Index.prototype.remove = function (doc) {
  var docRef = doc[this._ref],
      docTokens = this.documentStore.get(docRef)

  this.documentStore.remove(docRef)

  docTokens.forEach(function (token) {
    this.tokenStore.remove(token, docRef)
  }, this)
}

If the document doesn't exists, docTokens get null, so the forEach method fails. I think it should be checked and if so, ignored.

olivernn commented 11 years ago

Good catch. There should definitely be a check that a document with that reference already exists before trying to remove it.

The document store already has a method for checking whether or not a ref exists, it just isn't being used here. A fix should just be a case of checking the documentStore for the docRef before retrieving the docTokens.

if (!this.documentStore.has(docRef)) return

Feel free to open a pull request with a fix for this if you want, don't worry about updating the docs or the build, you should be able to add a test to test/index_test.js and add your fix in lib/index.js. Either that or I'll make a fix tomorrow and push a new version.

piranna commented 11 years ago

Ok, I'll try to take a look on it.

2013/4/24 Oliver Nightingale notifications@github.com

Good catch. There should definitely be a check that a document with that reference already exists before trying to remove it.

The document store already has a method for checking whether or not a ref exists, it just isn't being used here. A fix should just be a case of checking the documentStore for the docRef before retrieving the docTokens.

if (!this.documentStore.has(docRef)) return

Feel free to open a pull request with a fix for this if you want, don't worry about updating the docs or the build, you should be able to add a test to test/index_test.js and add your fix in lib/index.js. Either that or I'll make a fix tomorrow and push a new version.

— Reply to this email directly or view it on GitHubhttps://github.com/olivernn/lunr.js/issues/22#issuecomment-16963869 .

"Si quieres viajar alrededor del mundo y ser invitado a hablar en un monton de sitios diferentes, simplemente escribe un sistema operativo Unix." – Linus Tordvals, creador del sistema operativo Linux

olivernn commented 11 years ago

Hey, hows it going with your patch? Let me know if you don't have the time to do it and I'll make the change…

piranna commented 11 years ago

Sorry, I've been busy this days :-/ If required I can do it this afternoon...

By the way, the search engine is working great on my app ;-) El 30/04/2013 17:03, "Oliver Nightingale" notifications@github.com escribió:

Hey, hows it going with your patch? Let me know if you don't have the time to do it and I'll make the change…

— Reply to this email directly or view it on GitHubhttps://github.com/olivernn/lunr.js/issues/22#issuecomment-17232660 .

olivernn commented 11 years ago

No massive rush, just wanted to make sure you were still keen on contributing a patch. Glad lunr is working well for you!

piranna commented 11 years ago

I've just send you a pull request from my project account ;-)

olivernn commented 11 years ago

This has been fixed in version 0.3.1