klesh commented on code in PR #5506:
URL:
https://github.com/apache/incubator-devlake/pull/5506#discussion_r1232425557
##########
backend/helpers/pluginhelper/api/connection_helper.go:
##########
@@ -101,8 +104,16 @@ func (c *ConnectionApiHelper) List(connections
interface{}) errors.Error {
}
// Delete connection
-func (c *ConnectionApiHelper) Delete(connection interface{}) errors.Error {
- return CallDB(c.db.Delete, connection)
+func (c *ConnectionApiHelper) Delete(plugin string, connection interface{})
(*services.BlueprintProjectPairs, errors.Error) {
+ connectionId := reflectField(connection, "ID").Uint()
+ referencingBps, err := c.bpManager.GetBlueprintsByConnection(plugin,
connectionId)
+ if err != nil {
+ return nil, err
+ }
+ if len(referencingBps) > 0 {
+ return services.NewBlueprintProjectPairs(referencingBps),
errors.Conflict.New("Found one or more references to this connection")
+ }
+ return nil, CallDB(c.db.Delete, connection)
Review Comment:
> > also need to check if there are scopes referencing the connection
>
> @klesh I think we should allow users to delete a connection that has data
scope(s), as long as the connection and its data scopes are not referenced by
any project or bp.
>
> If not, deleting a connection with 10 repos will take 11 steps (deleting
10 repos and 1 connection) and more clicks to complete, which is dreadful.
>
> In this case, when deleting a connection, the frontend can loop the data
scope(s) and delete both the data scopes under this connection and the
connection itself.
@Startrekzky I agree with you, the Product Design is fine, no problem.
However, this API BUG has NO conflict with your description. All I'm saying is
"the API should make sure all its scopes get deleted before deleting the
connection". the deletion will be carried out without problem if the FE had
deleted the scopes.
@keon94 Please think about it, what if users request the API directly with
`curl`??, it would leave us some dangling scope and data. Our API should
function properly with or without `config-ui` Do NOT trust the API requests
EVER. It would be better to use a transaction with a proper lock.
I don't agree with the idea of letting FE delete scopes, but we could use it
as a temporary measure:
1. Race Condition: what if one adds scope to the connection while the other
deletes it?
2. Deleting Scope and related data could take a long time, what if users
close the browser during deletion?
3. Sooner or later, we must have GetScopesList paginated, it will add extra
complication to the FE.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
