[GitHub] [incubator-devlake] klesh commented on a diff in pull request #5153: [feat-4762] part1: Minimal support for deleting scopes

["klesh (via GitHub)" <gi...@apache.org>]

klesh commented on code in PR #5153:
URL: https://github.com/apache/incubator-devlake/pull/5153#discussion_r1190801860


##########
backend/helpers/pluginhelper/api/api_rawdata.go:
##########
@@ -37,16 +38,23 @@ type RawData struct {
 	CreatedAt time.Time
 }
 
+type TaskOptions interface {
+	GetParams() any
+}
+
 // RawDataSubTaskArgs FIXME ...
 type RawDataSubTaskArgs struct {
 	Ctx plugin.SubTaskContext
 
 	//	Table store raw data
 	Table string `comment:"Raw data table name"`
 
-	//	This struct will be JSONEncoded and stored into database along with raw data itself, to identity minimal
-	//	set of data to be process, for example, we process JiraIssues by Board
-	Params interface{} `comment:"To identify a set of records with same UrlTemplate, i.e. {ConnectionId, BoardId} for jira entities"`
+	// Deprecated: Use Options instead

Review Comment:
   Please explain the reason behind it, I don't understand.



##########
backend/helpers/pluginhelper/api/scope_helper.go:
##########
@@ -207,24 +243,53 @@ func (c *ScopeApiHelper[Conn, Scope, Tr]) GetScopeList(input *plugin.ApiResource
 	if err != nil {
 		return nil, err
 	}
+	if params.loadBlueprints {
+		if len(scopeIdFieldName) == 0 {
+			return nil, errors.Default.New("scope Id field name is not known") //temporary, limited solution until I properly refactor all of this in another PR
+		}
+		scopesById := c.mapByScopeId(apiScopes, scopeIdFieldName[0])
+		var scopeIds []string
+		for id := range scopesById {
+			scopeIds = append(scopeIds, id)
+		}
+		blueprintMap, err := c.bpManager.GetBlueprintsByScopes(scopeIds...)

Review Comment:
   Is this correct? I believe the `connectionId` is missing here.



##########
backend/helpers/pluginhelper/api/scope_helper.go:
##########
@@ -245,9 +310,95 @@ func (c *ScopeApiHelper[Conn, Scope, Tr]) GetScope(input *plugin.ApiResourceInpu
 			return nil, errors.NotFound.New("transformationRule not found")
 		}
 	}
-	scopeRes := &ScopeRes[Scope]{scope, reflect.ValueOf(rule).FieldByName("Name").String()}
+	scopeRes := &ScopeRes[Scope]{
+		Scope:                  scope,
+		TransformationRuleName: reflect.ValueOf(rule).FieldByName("Name").String(),
+	}
 	return &plugin.ApiResourceOutput{Body: scopeRes, Status: http.StatusOK}, nil
 }
+func (c *ScopeApiHelper[Conn, Scope, Tr]) DeleteScope(input *plugin.ApiResourceInput, scopeIdFieldName string, rawScopeParamName string,
+	getScopeParamValue func(db dal.Dal, scopeId string) (string, errors.Error)) (*plugin.ApiResourceOutput, errors.Error) {
+	params := c.extractFromDeleteReqParam(input)
+	if params == nil || params.connectionId == 0 {
+		return nil, errors.BadInput.New("invalid path params: \"connectionId\" not set")
+	}
+	if len(params.scopeId) == 0 || params.scopeId == "0" {
+		return nil, errors.BadInput.New("invalid path params: \"scopeId\" not set/invalid")
+	}
+	err := c.VerifyConnection(params.connectionId)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error verifying connection for connection ID %d", params.connectionId))
+	}
+	db := c.db
+	blueprintsMap, err := c.bpManager.GetBlueprintsByScopes(params.scopeId)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error retrieving scope with scope ID %s", params.scopeId))
+	}
+	blueprints := blueprintsMap[params.scopeId]
+	// find all tables for this plugin
+	tables, err := getPluginTables(params.plugin)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error getting database tables managed by plugin %s", params.plugin))
+	}
+	// delete all the plugin records referencing this scope
+	if rawScopeParamName != "" {
+		scopeParamValue := params.scopeId
+		if getScopeParamValue != nil {
+			scopeParamValue, err = getScopeParamValue(c.db, params.scopeId) // this function is optional - use it if API data params stores a value different to the scope id (e.g. github plugin)
+			if err != nil {
+				return nil, errors.Default.Wrap(err, fmt.Sprintf("error extracting scope parameter name for scope %s", params.scopeId))
+			}
+		}
+		for _, table := range tables {
+			err = db.Exec(createDeleteQuery(table, rawScopeParamName, scopeParamValue))
+			if err != nil {
+				return nil, errors.Default.Wrap(err, fmt.Sprintf("error deleting data bound to scope %s for plugin %s", params.scopeId, params.plugin))
+			}
+		}

Review Comment:
   I think we have to delete related records from the `_devlake_collector_latest_state` table as well



##########
backend/helpers/pluginhelper/api/scope_helper.go:
##########
@@ -245,9 +310,95 @@ func (c *ScopeApiHelper[Conn, Scope, Tr]) GetScope(input *plugin.ApiResourceInpu
 			return nil, errors.NotFound.New("transformationRule not found")
 		}
 	}
-	scopeRes := &ScopeRes[Scope]{scope, reflect.ValueOf(rule).FieldByName("Name").String()}
+	scopeRes := &ScopeRes[Scope]{
+		Scope:                  scope,
+		TransformationRuleName: reflect.ValueOf(rule).FieldByName("Name").String(),
+	}
 	return &plugin.ApiResourceOutput{Body: scopeRes, Status: http.StatusOK}, nil
 }
+func (c *ScopeApiHelper[Conn, Scope, Tr]) DeleteScope(input *plugin.ApiResourceInput, scopeIdFieldName string, rawScopeParamName string,
+	getScopeParamValue func(db dal.Dal, scopeId string) (string, errors.Error)) (*plugin.ApiResourceOutput, errors.Error) {
+	params := c.extractFromDeleteReqParam(input)
+	if params == nil || params.connectionId == 0 {
+		return nil, errors.BadInput.New("invalid path params: \"connectionId\" not set")
+	}
+	if len(params.scopeId) == 0 || params.scopeId == "0" {
+		return nil, errors.BadInput.New("invalid path params: \"scopeId\" not set/invalid")
+	}
+	err := c.VerifyConnection(params.connectionId)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error verifying connection for connection ID %d", params.connectionId))
+	}
+	db := c.db
+	blueprintsMap, err := c.bpManager.GetBlueprintsByScopes(params.scopeId)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error retrieving scope with scope ID %s", params.scopeId))
+	}
+	blueprints := blueprintsMap[params.scopeId]
+	// find all tables for this plugin
+	tables, err := getPluginTables(params.plugin)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error getting database tables managed by plugin %s", params.plugin))
+	}
+	// delete all the plugin records referencing this scope
+	if rawScopeParamName != "" {

Review Comment:
   There is a catch, the `webhook` plugin doesn't have `scopes` only `connections`.
   



##########
backend/helpers/pluginhelper/api/scope_helper.go:
##########
@@ -245,9 +310,95 @@ func (c *ScopeApiHelper[Conn, Scope, Tr]) GetScope(input *plugin.ApiResourceInpu
 			return nil, errors.NotFound.New("transformationRule not found")
 		}
 	}
-	scopeRes := &ScopeRes[Scope]{scope, reflect.ValueOf(rule).FieldByName("Name").String()}
+	scopeRes := &ScopeRes[Scope]{
+		Scope:                  scope,
+		TransformationRuleName: reflect.ValueOf(rule).FieldByName("Name").String(),
+	}
 	return &plugin.ApiResourceOutput{Body: scopeRes, Status: http.StatusOK}, nil
 }
+func (c *ScopeApiHelper[Conn, Scope, Tr]) DeleteScope(input *plugin.ApiResourceInput, scopeIdFieldName string, rawScopeParamName string,
+	getScopeParamValue func(db dal.Dal, scopeId string) (string, errors.Error)) (*plugin.ApiResourceOutput, errors.Error) {
+	params := c.extractFromDeleteReqParam(input)
+	if params == nil || params.connectionId == 0 {
+		return nil, errors.BadInput.New("invalid path params: \"connectionId\" not set")
+	}
+	if len(params.scopeId) == 0 || params.scopeId == "0" {
+		return nil, errors.BadInput.New("invalid path params: \"scopeId\" not set/invalid")
+	}
+	err := c.VerifyConnection(params.connectionId)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error verifying connection for connection ID %d", params.connectionId))
+	}
+	db := c.db
+	blueprintsMap, err := c.bpManager.GetBlueprintsByScopes(params.scopeId)
+	if err != nil {
+		return nil, errors.Default.Wrap(err, fmt.Sprintf("error retrieving scope with scope ID %s", params.scopeId))
+	}
+	blueprints := blueprintsMap[params.scopeId]
+	// find all tables for this plugin
+	tables, err := getPluginTables(params.plugin)

Review Comment:
   the function name is confusing, I suggest to rename it to `getAffectedTables`



-- 
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: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org