Skip to content

Bugfix/rdmp 359 internal catalogue bugs #2311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JFriel merged 9 commits into from
Feb 9, 2026
Merged

Bugfix/rdmp 359 internal catalogue bugs #2311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9470a4a
add drag and drop fix for internal catalogues
JFriel Feb 9, 2026
41884bc
prevent commit of internal catalogues
JFriel Feb 9, 2026
94199b1
add changelog
JFriel Feb 9, 2026
11d0480
fix typo
JFriel Feb 9, 2026
56ffdbb
add internal cohort test
JFriel Feb 9, 2026
ba9c21e
update test
JFriel Feb 9, 2026
95fcc16
fix shared assembly number
JFriel Feb 9, 2026
c6a2757
Merge branch 'develop' into bugfix/RDMP-359-internal-catalogue-bugs
JFriel Feb 9, 2026
18d6876
Update CHANGELOG.md
JFriel Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [9.2.0] - Unreleased
- Add Internal Note to Catalogue
- Fix issue with using Internal Catalogues in Cohort Identification Configurations

## [9.1.2] - Unreleased
- Automatically fetch user settings from previous versions of RDMP when installing the latest version
Expand Down
20 changes: 20 additions & 0 deletions Rdmp.Core.Tests/CohortCreation/CohortCompilerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,24 @@ public void TestCompilerAddAllTasks(TestCompilerAddAllTasksTestCase testCase, bo
aggregate5.DeleteInDatabase();
}
}

[Test]
public void TestCompilerWithInternalCatalogues()
{
var compiler = new CohortCompiler(cohortIdentificationConfiguration);
testData.catalogue.IsInternalDataset = true;
testData.catalogue.SaveToDatabase();

rootcontainer.AddChild(aggregate1, 1);
rootcontainer.AddChild(container1);
container1.Order = 2;
container1.SaveToDatabase();

cohortIdentificationConfiguration.RootCohortAggregateContainer_ID = rootcontainer.ID;
cohortIdentificationConfiguration.SaveToDatabase();

compiler.AddAllTasks(true);
Assert.That(compiler.Tasks.Keys.Select(k => k.State).Where(s => s == CompilationState.Crashed).ToList(), Has.Count.EqualTo(3));
Assert.That(compiler.Tasks.Keys.Where(s => s.State == CompilationState.Crashed).Select(k => k.CrashMessage.Message).ToList().Any(m => m.Contains(" is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations.")));
}
}
22 changes: 22 additions & 0 deletions Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,28 @@
task.State = CompilationState.Crashed;
}

if(task.Child is CohortAggregateContainer cac)
{
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;
}
}
Comment on lines +325 to +333

Check notice

Code scanning / CodeQL

Missed opportunity to use Where Note

This foreach loop
implicitly filters its target sequence
Loading
- consider filtering the sequence explicitly using '.Where(...)'.

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.

Suggested changeset 1
Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs b/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs
--- a/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs
+++ b/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs
@@ -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;
             }
         }
 
EOF
@@ -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;
}
}

Copilot is powered by AI and may make mistakes. Always verify output.
}

if (task.Child is AggregateConfiguration ac)
{
if (ac.Catalogue != null && ac.Catalogue.IsInternalDataset)
{
task.CrashMessage = new ArgumentException($"Catalogue {ac.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations.");
task.State = CompilationState.Crashed;
}
}

task.Log = queryBuilder?.Results?.Log;


Expand Down
6 changes: 6 additions & 0 deletions ...dExecution/AtomicCommands/ExecuteCommandAddCatalogueToCohortIdentificationSetContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ public ExecuteCommandAddCatalogueToCohortIdentificationSetContainer(IBasicActiva
{
_catalogueCombineable = new CatalogueCombineable(catalogue);

if(catalogue.IsInternalDataset)
{
SetImpossible($"Catalogue '{catalogue}' is an Internal dataset and cannot be added to a Cohort Identification Set Container");
return;
}

if (identifierColumn != null)
{
if (!identifierColumn.IsExtractionIdentifier)
Expand Down
2 changes: 2 additions & 0 deletions Rdmp.UI/CommandExecution/Proposals/ProposeExecutionWhenTargetIsAggregateConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Rdmp.Core.CommandExecution.AtomicCommands;
using Rdmp.Core.CommandExecution.Combining;
using Rdmp.Core.Curation.Data.Aggregation;
using Rdmp.Core.Curation.Data.Cohort;
using Rdmp.UI.AggregationUIs.Advanced;
using Rdmp.UI.CommandExecution.AtomicCommands;
using Rdmp.UI.ItemActivation;
Expand Down Expand Up @@ -55,6 +56,7 @@ public override ICommandExecution ProposeExecution(ICombineToMakeCommand cmd,
BasedOn = efps.ParameterSet.ExtractionFilter,
ParameterSet = efps.ParameterSet
},
CatalogueCombineable catalogueCombineable => new ExecuteCommandAddCatalogueToCohortIdentificationSetContainer(ItemActivator,targetAggregateConfiguration.GetCohortAggregateContainerIfAny(),catalogueCombineable.Catalogue),
_ => null
};
}
Expand Down
1 change: 1 addition & 0 deletions Rdmp.UI/CommandExecution/Proposals/ProposeExecutionWhenTargetIsCohortAggregateContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public override ICommandExecution ProposeExecution(ICombineToMakeCommand cmd,
//source is catalogue
case CatalogueCombineable sourceCatalogueCombineable:
{
if (sourceCatalogueCombineable.Catalogue.IsInternalDataset) return null;
if (sourceCatalogueCombineable.Catalogue.IsProjectSpecific(ItemActivator.RepositoryLocator.DataExportRepository))
{
var dx = (DataExportChildProvider)ItemActivator.CoreChildProvider;
Expand Down
Loading