fixing default connection behavior #234
Conversation
| @@ -170,7 +170,7 @@ protected void putObject(byte[] key, T object) { | |||
| * @param name to be mangled. | |||
| * @return mangled name. | |||
| */ | |||
| protected String mangle(String name) { | |||
There was a problem hiding this comment.
update the javadoc for this too
| .filter(c -> connectionId.equals(store.getConnectionId(c.getName()))).findFirst().isPresent(); | ||
| } | ||
|
|
||
| private Optional<Connection> getDefaultConnection(List<Connection> connections, |
There was a problem hiding this comment.
in this case, I think it's easier to just return null instead of using an Optional, as you end up converting to null anyway.
| @@ -252,6 +276,11 @@ public void list(HttpServiceRequest request, HttpServiceResponder responder, | |||
| public void delete(HttpServiceRequest request, HttpServiceResponder responder, | |||
| @PathParam("id") final String id) { | |||
| try { | |||
| if (isDefaultConnection(id)) { | |||
| responder.sendError(HttpURLConnection.HTTP_BAD_REQUEST, | |||
There was a problem hiding this comment.
I think it makes more sense for this to be a not authorized error
| } catch (Exception e) { | ||
| error(responder, e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private boolean isDefaultConnection(String connectionId) { |
There was a problem hiding this comment.
the use of 'default' is confusing because there is an actual single default connection. A better name could be 'canBeDeleted', as that's really what it's being used for.
I also think it's more clear if you just put the logic in the delete method instead of having a separate method, since it's basically a single line and is only called from one place.
| @@ -252,6 +276,11 @@ public void list(HttpServiceRequest request, HttpServiceResponder responder, | |||
| public void delete(HttpServiceRequest request, HttpServiceResponder responder, | |||
| @PathParam("id") final String id) { | |||
| try { | |||
| if (isDefaultConnection(id)) { | |||
| responder.sendError(HttpURLConnection.HTTP_BAD_REQUEST, | |||
| String.format("Cannot delete default connection %s", id)); | |||
There was a problem hiding this comment.
default is an ambiguous term here. I think we can say 'Cannot delete admin controlled connection %s.'
…eting default connection
gsps1
force-pushed
the
fix/default-connection-exists-check
branch
from
9597e95
to
0abe0cc
Compare
When listing connections, check if the configured default connection for ui landing exists in connection list.
When deleting a connection, check if its configured as a default connection and disallow deleting a connection if it is a default connection.
Reasoning behind this :
We create default connections during service handler initialize, however the initialize gets called on each handler thread initialization, which can happen quite often, hence even if a default connection is deleted, it gets immediately recreated. Also we don't want to store the state of default connection creation/deletion in a table to workaround that, as the admin can make changes to the config to make certain default connections as non-default and at that time we want (and will) to allow those connections to be deletable.
If there was a lifecycle phase such as prepareRun in service that runs only once, it would have been ideal to create the default connections during that phase. the default connection would gets recreated on DataPrep re-initialization if a user has deleted it would have been ideal. However we don't have that option currently.
This trade-off of disallowing users to delete a default connection isn't too bad as if the users doesn't want a default connection, they can contact the administrators about changing the app config for DataPrep and it can be removed from the config.
JIRA : https://issues.cask.co/browse/CDAP-14270