Bugfix/rdmp 359 internal catalogue bugs#2311
Conversation
| foreach (var cacac in cac.GetAggregateConfigurations()) | ||
| { | ||
| if (cacac.Catalogue != null && cacac.Catalogue.IsInternalDataset) | ||
| { | ||
| task.CrashMessage = new ArgumentException($"Catalogue {cacac.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations."); | ||
| task.State = CompilationState.Crashed; | ||
| break; | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix a "missed opportunity to use Where" in a foreach loop that immediately tests each element with an if and either continues or breaks, you replace the loop with a LINQ query that directly expresses the predicate, then act on the filtered result(s). This usually reduces nesting and makes it obvious you are searching or filtering, rather than performing arbitrary per-element logic.
For this specific case, the loop is only interested in whether there exists any aggregate configuration whose Catalogue is non-null and IsInternalDataset. On finding the first such configuration, it sets task.CrashMessage and task.State and stops iterating. The clearest equivalent using LINQ is to obtain the first offending configuration via FirstOrDefault, then if that result is non-null, set CrashMessage/State. We already have using System.Linq; at the top, so no new imports are needed. The behavior remains unchanged: there is at most one place where CrashMessage/State are set in this block, and enumeration stops as soon as the first problematic configuration is found.
Concretely, in Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs, replace the foreach block at lines 325–333 with code that calls cac.GetAggregateConfigurations().FirstOrDefault(...) using the same predicate (ac => ac.Catalogue != null && ac.Catalogue.IsInternalDataset). If a matching configuration is found, construct the same ArgumentException using that configuration’s Catalogue.Name and set task.State = CompilationState.Crashed;. No additional methods or fields are required.
| @@ -322,14 +322,14 @@ | ||
|
|
||
| if(task.Child is CohortAggregateContainer cac) | ||
| { | ||
| foreach (var cacac in cac.GetAggregateConfigurations()) | ||
| var offendingConfig = cac | ||
| .GetAggregateConfigurations() | ||
| .FirstOrDefault(ac => ac.Catalogue != null && ac.Catalogue.IsInternalDataset); | ||
|
|
||
| if (offendingConfig != null) | ||
| { | ||
| if (cacac.Catalogue != null && cacac.Catalogue.IsInternalDataset) | ||
| { | ||
| task.CrashMessage = new ArgumentException($"Catalogue {cacac.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations."); | ||
| task.State = CompilationState.Crashed; | ||
| break; | ||
| } | ||
| task.CrashMessage = new ArgumentException($"Catalogue {offendingConfig.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations."); | ||
| task.State = CompilationState.Crashed; | ||
| } | ||
| } | ||
|
|
Proposed Change
Prevent internal catalogues from beign drag-and-dropped into CICs
Prevent cohorts for being executed that have internal catalogues
Type of change
What types of changes does your code introduce? Tick all that apply.
Checklist
By opening this PR, I confirm that I have: