search

appscot / sails-orientdb

OrientDB adapter for Waterline / Sails.js ORM
MIT License
25 stars 23 forks source link

Proposed fix for issue #131 #133

Closed nidaca closed 8 years ago

nidaca commented 8 years ago

This fix solves https://github.com/appscot/sails-orientdb/issues/131 issue by removing from execution the queries that doesn't have all parameters well defined and valid (parameters must be not undefined) ...

dmarcelino commented 8 years ago

Hi @nidaca, thanks for contributing. Where you able to understand what was the root cause of the undefined params? In other words, is it a user issue or a sails-orientdb issue? My concern is that this solution may actually be hiding the real issue instead of addressing it.

nidaca commented 8 years ago

Hello @dmarcelino, thank you for the quick feedback on this. The undefined parameter is generated by waterline/sails-orientdb when trying to populate a non-existent (optional) LINK value of a vertex, as in the description of the problem (see the avatar attribute). Actually the parameter is the @rid value of the second generated query: SELECT ... FROM File WHERE @rid=:param0. The undefined value is made by the join / populate logic, no user contribution in this ;) ... I didn't identified the exact location of the created value, is somewhere in the join logic ... in the same time I'm thinking that an extra validation of parameters on the other "side of the chain", before oriento gets into action, is actually a non-harmful safety measure that will prevent this kind of unwanted behavior caused by a broken piece of input generated from a top module (or even from a user input). What do you think about this ?

nidaca commented 8 years ago

... obviously, extra investigations needs to be done in order to find the piece of code that generates this particular broken input.

dmarcelino commented 8 years ago

Hey @nidaca, can you create an automated test that fails without your changes (and passes with them)?

nidaca commented 8 years ago

Hi @dmarcelino, next days when I'll have some time for this I'll make a test or even investigate it further to see the source of the problem.

nidaca commented 8 years ago

hi @dmarcelino , managed to make a short test for the issue, but I didn't have the time to check the source of the problem ...

nidaca commented 8 years ago

hey @dmarcelino, should I close this PR ? the bug was recently fixed in waterline-cursor@0.0.6

dmarcelino commented 8 years ago

Hey @nidaca, the test is still useful, can you keep it and remove only the code changes?

nidaca commented 8 years ago

sure @dmarcelino, it's done ...

dmarcelino commented 8 years ago

Cool, thanks!