fogine / couchbase-odm

CouchbaseODM is a promise-based Node.js ODM for Couchbase
https://fogine.github.io/couchbase-odm
MIT License
12 stars 3 forks source link

error throwed when id not match predefined pattern #12

Closed jurajsimbs closed 8 years ago

jurajsimbs commented 8 years ago

example code:

couchbase.modelManager.get('PamelaAnderson').getById('__not-uuid-key__').then(function (pamelaUser) {
            //show naked pamela picture    
}).catch(function (pamelaLoadError) {
           //here I expected error about key not in proper format
})

...but instead manager.get method threw new KeyError and my code exploded into my face

Is this like expected behavior? Shouldn't all the errors during get call be routed to promise catch ?

because now I'm forced to have $#@%#!@!$#@ ugly code like this

try {
   couchbase.modelManager.get('PamelaAnderson').getById('__not-uuid-key__').then(function     (pamelaUser) {
               //show naked pamela picture    
   }).catch(function (pamelaLoadError) {
              //here I expected error about key not in proper format
   })
} catch (pamelaFatalError) {
   //oh no there was some horrible problem during pamela load
}

I'm confused

fogine commented 8 years ago

Hello, to be honest, the issue does not make much sense to me right now, could you clarify? The getById method returns always rejected Promise if an error occurs. I tried to call Model.getById('non-valid-uuid-key') and I got rejected Promise with The key does not exist on the server which is expected behavior.

However the ModelManager.get('modelName') method is fully synchronous method thus it throws ModelNotFoundError if a Model with provided name does not exist.

This works for me:

try {
   var Model = couchbase.modelManager.get('PamelaAnderson');
} catch (err) {
  // no model with name 'PamelaAnderson' was found
}

Model.getById('__not-uuid-key__').then(function(pamelaUser) {
        //success
}).catch(function (pamelaLoadError) {
       // model not found
});

Note that there is no checking for validity of an id in the getById method. if you provided incorrect id you will get StorageError: The key does not exist on the server.

The check for validity of an id is made only at times when you provide whole document's key (not just an id) and we need to parse the key string for the id. Then, parsed id is checked that it's in valid format.

jurajsimbs commented 8 years ago

hmm that is strange... When I run

Model.getById('__not-uuid-key__').then(function(pamelaUSer) {}).catch(function(err){})

I got exception -> and it is not in .catch() method...

...I'll try to put up some example for you to try...

fogine commented 8 years ago

Oh, I see now. In the develop branch, there is indeed added checking for validity of the format of an id - Exception is not handled properly in Model.getById method and thus it's propagated up the call chain. There is similar bug in the master branch as well.

Thanks for the report!