Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow valid set() calls without a callback to proceed. These were fa… #249

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ exports.validateArg = function validateArg (args, config) {
break;

case Function:
if (toString.call(value) !== '[object Function]') {
if (typeof obj !== "undefined" && toString.call(value) !== '[object Function]') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateArg is supposed to confirm that an argument is a particular type. It seems like a bad place to turn all function arguments into optionals system wide. There must be a better place for this, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ grep -r validateArg *
lib/memcached.js: if (query.validate && !Utils.validateArg(query, this)) {
lib/utils.js:exports.validateArg = function validateArg (args, config) {

"system wide" seems to be this one call inside of "command", so in all cases that I'm aware of, it is the callback. So in all of those cases (globally?), "undefined" is actually a valid value for that argument. Perhaps renaming "validateArg" to "validateCommandArg" would make that clearer if you still feel strongly about it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my point is that if it's optional, you may want to check for optionality before calling this function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, passed functions are always callbacks, so they should be globally optional. I have been unable to find any place where they are not, so that seems to me that would be writing code support for a case that doesn't exist. I'm not intimately familiar with the module though, so please correct me if you know of one.

If there is one, IMO, simply checking for optionality before the call would require that I then pass a "fake" function to validateArg in order to get it to pass which seems very hacky. If this is absolutely necessary I would add another item to the config called "OptionalFunction" and copy/paste the "Function" code except also allowing undefined, and then globally find/replace Function with OptionalFunction in every command config definition.

But again, unless a case is identified, this seems like code bloat for a situation that doesn't exist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdunavant I understand your case. I'm also not 100% intimately familiar with the entire codebase however.
@3rd-Eden, any input?

err = 'Argument "' + key + '" is not a valid Function.';
}

Expand Down
37 changes: 37 additions & 0 deletions test/memcached-get-set.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ var assert = require('assert')

global.testnumbers = global.testnumbers || +(Math.random(10) * 1000000).toFixed();

var set_without_callback_testnr = ++global.testnumbers;

/**
* Expresso test suite for all `get` related
* memcached commands
Expand Down Expand Up @@ -625,4 +627,39 @@ describe("Memcached GET SET", function() {
done();
});
});

/*
Make sure that set without a callback works.
While not common, sometimes people want to just call set without a callback and ignore error cases. While this is a bad practice, there is no requirement that you pass in a callback for most functions. The code used to silently fail without actually setting the value (since it would try to error using the callback, which did not exist, so it would just ignore and keep going.
*/
before(function(done) {
var memcached = new Memcached(common.servers.single)
, message = 'i was set'
, testnr = set_without_callback_testnr;

memcached.set('set_without_callback_test:'+testnr , message, 1000 );
// wait .5 seconds before continuing. While this is an arbitrary value, if your memcache set takes half a second you have bigger issues
setTimeout(done, 500);
});
it("make sure you can set() without a callback", function(done) {
var memcached = new Memcached(common.servers.single)
, message = 'i was set'
, testnr = set_without_callback_testnr
, callbacks = 0;

memcached.get('set_without_callback_test:'+testnr, function(error, answer) {
++callbacks;

assert.ok(!error);
memcached.end();
assert.equal(callbacks, 1);

assert.ok(typeof answer === 'string');
answer.should.eql(message);

done();
});
});


});