Remove corresponding issues on branch delete #1224

AAAlinaaa · 2023-09-29T14:33:55Z

No description provided.

codecov · 2023-09-29T15:10:17Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (e57082a) 47.96% compared to head (c96bc96) 48.00%.
Report is 3 commits behind head on 9.x.

Files	Patch %	Lines
.../snowowl/core/branch/ValidationCleanupService.java	67.56%	11 Missing and 1 partial ⚠️
...ional/snowowl/core/branch/BranchDeleteRequest.java	83.33%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                9.x    #1224      +/-   ##
============================================
+ Coverage     47.96%   48.00%   +0.04%     
- Complexity    13831    13852      +21     
============================================
  Files          1942     1943       +1     
  Lines         94946    94989      +43     
  Branches      10955    10957       +2     
============================================
+ Hits          45541    45600      +59     
+ Misses        46378    46360      -18     
- Partials       3027     3029       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmark · 2023-11-13T10:01:00Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+		removeStaleIssues(context, List.of(resourceURI), previousAnalysisRunIds);
+	}
+
+	public void removeStaleIssues(ServiceProvider context, List<String> resourceURIs, List<String> previousAnalysisRunIds) {


Suggested change

public void removeStaleIssues(ServiceProvider context, List<String> resourceURIs, List<String> previousAnalysisRunIds) {

public void removeStaleIssues(ServiceProvider context, List<String> resourceURIs, List<String> resultIds) {

cmark · 2023-11-13T10:01:07Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+		removeStaleIssues(context, resourceURIs, Collections.emptyList());
+	}
+
+	public void removeStaleIssues(ServiceProvider context, String resourceURI, List<String> previousAnalysisRunIds) {


Suggested change

public void removeStaleIssues(ServiceProvider context, String resourceURI, List<String> previousAnalysisRunIds) {

public void removeStaleIssues(ServiceProvider context, String resourceURI, List<String> resultIds) {

cmark · 2023-11-13T10:01:38Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+	public void removeStaleIssues(ServiceProvider context, List<String> resourceURIs, List<String> previousAnalysisRunIds) {
+		AsyncRequest<Boolean> request;
+
+		if (!previousAnalysisRunIds.isEmpty()) {


Suggested change

if (!previousAnalysisRunIds.isEmpty()) {

if (!CompareUtils.isEmpty(resultIds)) {

cmark · 2023-11-13T10:02:04Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+		} else {
+			request = ValidationRequests.issues()
+				.prepareDelete()
+				.setCodeSystemURIs(resourceURIs)


What happens if the resourceURIs is null or empty?

Nothing good, we shouldn't delete without resource uri filters.

Yeah, but that's a flaw in the API. If the caller should only use one input parameter, then we should split this method into two separate implementations instead of merging them into one with if else blocks.

cmark · 2023-11-13T10:02:37Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+				.buildAsync();
+		}
+
+		final String description = String.format("Remove validation issues on stale/removed branch(es) %s", Joiner.on(", ").join(resourceURIs));


Suggested change

final String description = String.format("Remove validation issues on stale/removed branch(es) %s", Joiner.on(", ").join(resourceURIs));

final String description = String.format("Remove validation issues on stale/removed branch(es) of '%s'", Joiner.on(", ").join(resourceURIs));

cmark · 2023-11-13T10:03:28Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+	public ValidationCleanupService() {
+	}
+
+	public void removeStaleIssues(ServiceProvider context, String resourceURI) {


Rename all these methods to scheduleStaleIssueRemoval since it is not actually removing anything from the index but schedules a job that removes it.

apeteri

👍 with a suggestion below.

apeteri · 2023-11-22T10:18:43Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+		JobRequests.prepareSchedule()
+			.setRequest(request)
+			.setDescription(description)
+			.setUser(context.service(User.class).getUserId())


This is the default behavior in ScheduleJobRequest, you can leave this property unset if you don't want to override the requester:

snow-owl/core/com.b2international.snowowl.core/src/com/b2international/snowowl/core/jobs/ScheduleJobRequest.java

Line 125 in 6b2f0c9

final String userId = !Strings.isNullOrEmpty(user) ? user : context.service(User.class).getUserId();

cmark · 2023-12-12T08:38:13Z

...ernational.snowowl.core/src/com/b2international/snowowl/core/branch/BranchDeleteRequest.java

+			try {
+				//Schedule a job to remove issues corresponding to this branch
+				TerminologyResource resource = context.service(PathTerminologyResourceResolver.class).resolve(context, context.info().id(), getBranchPath());
+				String resourceURI = resource.getResourceURI(getBranchPath()).toString();


It looks like we have to treat the new tilde path variants (XZX~branch) as well here. Could you please add support for that?

cmark · 2023-12-12T08:38:26Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

@@ -0,0 +1,79 @@
+package com.b2international.snowowl.core.branch;


Missing license and since tag.

cmark · 2023-12-12T08:40:04Z

...ional.snowowl.core/src/com/b2international/snowowl/core/branch/ValidationCleanupService.java

+		} else {
+			request = ValidationRequests.issues()
+				.prepareDelete()
+				.setCodeSystemURIs(resourceURIs)


Yeah, but that's a flaw in the API. If the caller should only use one input parameter, then we should split this method into two separate implementations instead of merging them into one with if else blocks.

AAAlinaaa self-assigned this Sep 29, 2023

cmark requested changes Nov 13, 2023

View reviewed changes

cmark requested a review from apeteri November 22, 2023 10:03

apeteri approved these changes Nov 22, 2023

View reviewed changes

AAAlinaaa added 10 commits December 6, 2023 14:25

fix(validation): Remove corresponding issues on branch delete

e8d187c

fix(validation): Refactor validation issue clean up

0100342

fix(validation): Fix log tracing

54788d6

fix(cleanup): Rename parameter

ddb1859

fix(cleanup): Update parameter name

487b222

fix(cleanup): Do not delete issues without resource/branch restrictions

e3ed550

fix(cleanup): Use CompareUtils for empty checks

eff0ef2

fix(cleanup): Fix job description

204b04f

fix(cleanup): Rename service methods

7f56487

fix(cleanup): Let default user be set for job request

8500f18

AAAlinaaa force-pushed the issue/SO-5533-clear-stale-validation-issues branch from dc8f309 to 8500f18 Compare December 6, 2023 17:07

cmark requested changes Dec 12, 2023

View reviewed changes

AAAlinaaa added 3 commits December 12, 2023 10:18

fix(cleanup): Organize import statements

f16b635

fix(cleanup): Add missing license header and since tag

0e9f56b

fix(cleanup): Call dedicated methods in the different delete scenarios

c96bc96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove corresponding issues on branch delete #1224

Remove corresponding issues on branch delete #1224

AAAlinaaa commented Sep 29, 2023

codecov bot commented Sep 29, 2023 •

edited

cmark Nov 13, 2023

cmark Nov 13, 2023

cmark Nov 13, 2023

cmark Nov 13, 2023

AAAlinaaa Dec 6, 2023

cmark Dec 12, 2023

cmark Nov 13, 2023

cmark Nov 13, 2023

apeteri left a comment

apeteri Nov 22, 2023

cmark Dec 12, 2023

cmark Dec 12, 2023

cmark Dec 12, 2023

	public void removeStaleIssues(ServiceProvider context, List<String> resourceURIs, List<String> previousAnalysisRunIds) {
	public void removeStaleIssues(ServiceProvider context, List<String> resourceURIs, List<String> resultIds) {

	if (!previousAnalysisRunIds.isEmpty()) {
	if (!CompareUtils.isEmpty(resultIds)) {

	final String description = String.format("Remove validation issues on stale/removed branch(es) %s", Joiner.on(", ").join(resourceURIs));
	final String description = String.format("Remove validation issues on stale/removed branch(es) of '%s'", Joiner.on(", ").join(resourceURIs));

		@@ -0,0 +1,79 @@
		package com.b2international.snowowl.core.branch;

Remove corresponding issues on branch delete #1224

Are you sure you want to change the base?

Remove corresponding issues on branch delete #1224

Conversation

AAAlinaaa commented Sep 29, 2023

codecov bot commented Sep 29, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apeteri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2023 •

edited