Adding feature for - "Test Connection" #532
Conversation
lib/marklogic.js
Outdated
| * Tests if a connection is successful. | ||
| * @method DatabaseClient#testConnection | ||
| * @since 2.1 | ||
| * @returns true if the connection is successful and false otherwise with an error message |
There was a problem hiding this comment.
Would it be more accurate to say that it returns an object with a connected property of true or false and, in the false case, httpStatusCode and httpStatusMessage properties identifying the failure?
lib/marklogic.js
Outdated
| requestOptions.method = 'HEAD'; | ||
| requestOptions.path = mlutil.databaseParam(connectionParams, '/v1/ping', '?'); | ||
| var operation = new Operation( | ||
| 'test operation', this, requestOptions, 'empty', 'single' |
There was a problem hiding this comment.
Should the last parameter be empty? After experimenting with curl, it looks like the HEAD request doesn't return a body in either the success or failure cases.
lib/marklogic.js
Outdated
| } else { | ||
| content = {connected: false, | ||
| httpStatusCode: this.responseStatusCode, | ||
| httpStatusMessage: operation.responseStatusMessage |
There was a problem hiding this comment.
Should the assignment use this.responseStatusMessage instead of operation.responseStatusMessage for consistency and to avoid side effects if the code is refactored to move the addOutputTransform() function out of the the testConnection() function in the future?
The this object refers to the object on which the function is called while operation is a closure on the local variable of testConnection()
lib/marklogic.js
Outdated
| content = {connected: false, | ||
| httpStatusCode: this.responseStatusCode, | ||
| httpStatusMessage: operation.responseStatusMessage | ||
| } |
There was a problem hiding this comment.
Should the previous line end with a semicolon to reinforce that it closes the object literal assigment starting on line 399?
lib/marklogic.js
Outdated
| } | ||
| var wrapper = {content: [content]}; | ||
| } | ||
| return wrapper; |
There was a problem hiding this comment.
I've two concerns:
- The wrapper variable is assigned only inside the else branch but returned from ddOutputTransform() in the if case as well. Wouldn't the return value be undef in that case?
- What is the rationale for wrapping the content value instead of just returning the content value?
test-basic/documents-quick.js
Outdated
| .result(function(response) { | ||
| flag = true; | ||
| }).then(function(){ | ||
| assert(flag == true); |
There was a problem hiding this comment.
Because the result() callback doesn't return a promise, I believe the then() callback will never execute.
Same comment on subsequent tests.
test-basic/documents-quick.js
Outdated
| var flag = false; | ||
| db.testConnection() | ||
| .result(function(response) { | ||
| flag = true; |
There was a problem hiding this comment.
Now that test connection returns an object in all cases, would a better test assert that response.connected is true?
test-basic/documents-quick.js
Outdated
| db1.testConnection() | ||
| .result(function(response) { | ||
| flag = true; | ||
| respContent = response.content[0]; |
There was a problem hiding this comment.
Instead of having to get a content subproperty, would it be better to have the properties directly on the response.
Same comment on the subsequent test.
There was a problem hiding this comment.
Thanks for adding this useful capability.
One last minute thought -- would it convey the intent better to call the method "checkConnection" instead of "testConnection"? I'm wondering if the specific connotations of "test" give people the wrong idea.
Please consider and, after resolving either way, merge the commit.
On to Java -- and then letting the documentation folk know to mention the check in the Node.js and Java guides.
|
Thanks Erik. ! |
|
The documentation on this still refers to "testConnection". |
|
Ping @ehennum. |
|
Please see Tom's feedback... |
Enhancement feature referring to - #346