check datastore connectivity after registering it#1621
check datastore connectivity after registering it#1621eashaw merged 3 commits intobalderdashy:masterfrom
Conversation
|
@eashaw may I please have your review on this PR? 🙇 |
lib/waterline.js
Outdated
| } | ||
|
|
||
| // Check if the datastore adapter has a `validateConnection` method | ||
| if (_.has(datastore.adapter, 'validateConnection')) { |
There was a problem hiding this comment.
Could you instead use getConnection and releaseConnection adapter methods (you'll still need to check if they both exist)
Heres an example of how you can access the manager
https://github.com/balderdashy/sails-hook-orm/blob/9c2a3c768eba152e47af645d02462ed831f87dbb/lib/build-registered-datastore-instance.js#L116
There was a problem hiding this comment.
@eashaw The idea here was to move the operations on connection objects (Such as getConnection and releaseConnection which are more common in the SQL realm and might not be applicable in other cases) away from waterline (and the orm hook) and keep it contained within the adapters (as done in balderdashy/sails-postgresql#290). This way each adapter can be more flexible in terms of how it wants to validate connectivity and waterline needn't be concerned with the implementation details of how connectivity to a datastore is to be validated. This is my opinion though, Let me know if you still want me to change this.
There was a problem hiding this comment.
Hey @bhargavkn, good news! getConnection and releaseConnection are standard across Waterline adapters, relational or not. https://github.com/node-machine/driver-interface/tree/e44cb443ae5cb16fdb824dc0a5bec3996563b1e1#connectable
There was a problem hiding this comment.
Great, will push the changes shortly. Thanks for sharing the doc.
bhargavkn
force-pushed
the
feature-test-datastore-connectivity
branch
from
ac1fe33 to
a7a5a6d
Compare
bhargavkn
force-pushed
the
feature-test-datastore-connectivity
branch
from
a7a5a6d to
3b64561
Compare
|
@eashaw Does this seem better? |
|
@eashaw Please let me know in case there are any pending action items on my part in order to merge this 🙇 |
2 participants
Changes in this PR attempt to resolve balderdashy/sails#7095 by making waterline
validateConnectionfor a datastore after it has been registered. This will make waterline return an error if a datastore was registered but waterline's attempt to validate the connection failed.