[GitHub] [tinkerpop] spmallette commented on issue #1187: TINKERPOP-2132 Authentication eagerly to avoid unauthorized responses

[GitBox <gi...@apache.org>]

spmallette commented on issue #1187: TINKERPOP-2132 Authentication eagerly to avoid unauthorized responses
URL: https://github.com/apache/tinkerpop/pull/1187#issuecomment-527587888
 
 
   > It basically follows the same idea as your previous work in the TINKERPOP-2132 branch, which has been there for a while, but is not merged for some reason I wander.
   
   I mentioned in the JIRA ticket why I didn't merge it. It introduced a breaking change to the Gremlin Server protocol to properly fix it and I didn't want to do that for 3.3.x or 3.4.x. I think it would be best if we could avoid that if at all possible as it means previous versions of the driver won't be able to connect along those release lines.
   
   > In this commit, I added a step that sends a validation request at the end of the constructor of Connection.
   
   So, I suppose this approach is a possible solution. You eagerly do the authentication operation of the `Connection` by way of a validation message. I guess that the `Connection` does the websocket handshake in the constructor, so immediately sending a validation message to force authentication might not have bad consequences. It's not an elegant solution, but if this problem is really bothering people and no other non-breaking solutions can be found then I suppose we could go with it ([assuming the tests](https://github.com/apache/tinkerpop/pull/1187#discussion_r320410110) show this fix actually does solve the problem).
   
   > not included in this commit in order to minimize the modifications.
   
   Thank you - that is appreciated.
   
   > The TimeoutException thrown when no host is available is a little bit misleading. Maybe a specialized NoHostAvailableException is better.
   
   I think i made a note about that elsewhere in my review. The `TimeoutException` isn't so good. 
   
   > It's a commonly used pattern to catch checked exceptions and wrap them in RuntimeExceptions and re-throw
   
   Yeah - I think the exception handling could be better. The generalized `RuntimeException` isn't so nice. It would be nice to make that better if we could introduce improvements without breaking changes, but that seems hard at this point. It might need to wait until 3.5.0...not sure.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services