[APP-367] feat: Update subset_sql to use basic filters from request by emyl3 · Pull Request #3160 · CDCgov/NEDSS-Modernization

emyl3 · 2026-04-13T21:52:51Z

Description

The PR takes basic filters from a ReportConfiguration then creates a WHERE clause and adds that to the subset_sql. The funcationality has been pulled from buildWhereClause() in ReportSQLBuilder. This only implements the basic filters.

Tickets

APP-367

Checklist before requesting a review

PR focuses on a single story
Code has been fully tested to meet acceptance criteria
PR is reasonably small and reviewable (Generally less than 10 files and 500 changed lines)
All new functions/classes/components reasonably small
Functions/classes/components focused on one responsibility
Code easy to understand and modify (clarity over concise/clever)
PRs containing TypeScript follow the Do's and Don'ts
PR does not contain hardcoded values (Uses constants)
All code is covered by unit or feature tests

…r loop

# Conflicts: # apps/modernization-api/src/main/java/gov/cdc/nbs/report/ReportService.java

mcmcgrath13 · 2026-04-22T16:24:53Z

+public class FieldFormatter {
+  public String formatField(String type, String value) {
+    return switch (type.toUpperCase()) {
+      case "STRING" -> "'" + value.replace("'", "''") + "'";


do we need to do any other sql escaping?

mcmcgrath13 · 2026-04-22T16:28:01Z

+
+    for (FilterConfiguration filter : reportConfig.filters()) {
+      findColumn(reportConfig, filter)
+          .ifPresent(


what about building filters that don't have columns? do all basic filters target a column?

The current NBS6 code callsgetDataSourceColumnDT() as one of the first steps. That method throws an exception if a column is not found. I was just working from that. Is that a safe route?

I think so. The advanced filter doesn't have a column, but I'm pretty sure every basic filter does and since this is for basic an error makes sense

mcmcgrath13 · 2026-04-22T16:28:24Z

+              column -> {
+                String segment = buildFilterFragment(filter, column);
+                if (!segment.isEmpty()) finalWhere.add(segment);
+              });


(q, b): should there be an error if there's no column?

yup, I'll change to throw an exception

mcmcgrath13

Overall structure looks good, but need to fix up the data source for the filter building

# Conflicts: # apps/modernization-api/src/main/java/gov/cdc/nbs/report/ReportService.java # apps/modernization-api/src/main/java/gov/cdc/nbs/report/models/AdvancedFilterConfiguration.java # apps/modernization-ui/src/generated/models/BasicFilterConfiguration.ts

mcmcgrath13 · 2026-05-06T22:56:07Z

+    assertThat(monthResult.get(1)).isEqualTo("'2024-02-29'"); // Leap year check
+
+    // Test yyyy Range
+    List<String> yearRange = List.of("2023", "2023");


Suggested change

List<String> yearRange = List.of("2023", "2023");

List<String> yearRange = List.of("2023", "2024");

to stress test leap years

mcmcgrath13 · 2026-05-06T22:57:02Z

+  @Test
+  void should_throw_exception_for_invalid_range_size() {
+    List<String> tooShort = List.of("2023");
+    assertThatThrownBy(() -> formatter.convertToSQLFromDateRange(tooShort))


can we also test too long?

mcmcgrath13 · 2026-05-06T22:59:18Z

+    String whereFragment =
+        mockWhereClauseService.buildBasicWhereFragment(reportConfig, basicFilterRequest);
+
+    assertThat(whereFragment).isEqualTo("([ColumnName] IN (condition1))");


Suggested change

assertThat(whereFragment).isEqualTo("([ColumnName] IN (condition1))");

assertThat(whereFragment).isEqualTo("([ColumnName] IN ('condition1'))");

mcmcgrath13 · 2026-05-06T23:01:47Z

+    mockWhereClauseService = new WhereClauseService(fieldFormatter);
+    // Default behavior for formatter: return value as is
+    Mockito.lenient()
+        .when(fieldFormatter.formatField(anyString(), anyString()))
+        .thenAnswer(ff -> ff.getArgument(1));


(q, nb): why do we need to mock the where clause service? it's stateless so couldn't we just use it? My gut-code-smell still doesn't really grok why the where clause service is a service and not just a class that's normally imported and used where it needs using (and then we wouldn't mock it, but just use it) - can you eli5?

same with the field formatter - seems like those both could be static classes

mcmcgrath13 · 2026-05-06T23:05:55Z

+import java.util.List;
+import org.springframework.stereotype.Component;
+
+@Component


(q, nb): why have this be a component instead of just a regular utility class?

mcmcgrath13 · 2026-05-11T13:08:04Z

-    filterType?: string;
-    filterName?: string;
+    codeSetName?: string;
+    type?: string;


(comm, b): The rest of the UI needs to be updated to use these new names, same with defaultValues

I think #3217 should address why the tests are still passing even though there're a lot of off references

think I got all these fixed

mcmcgrath13 · 2026-05-11T13:17:04Z

    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) Long reportFilterUid,
-    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) List<String> values) {}
+    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) List<String> values,
+    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) Boolean includeNulls) {}


(thought, nb): as we get around to the front end side of this work, we might want to reconsider the required-ness here since not all filters allow nulls, so it may be simpler to allow missingness as false

mcmcgrath13 · 2026-05-11T13:21:58Z

+    finalWhere.add(basicWhereFragment);
+
+    // Only return the WHERE clause if it contains anything beyond the initial "WHERE " prefix
+    return finalWhere.length() > 6 ? finalWhere.toString() : "";


(nit, nb): might be minor-ly more readable to pull out WHERE to a constant and then change this to finalWhere.equals(SQL_WHERE) and flip the ternary branches so then it's a more explicit check vs implicit with the comment. Though maybe that doesn't work easily with string joiners?

I like this. I already used a const for the AND so another for WHERE feels right. Added

mcmcgrath13 · 2026-05-11T13:28:50Z

+   * @param column The metadata for the column being filtered.
+   * @return A parenthesized SQL fragment.
+   */
+  private String buildBasicTimeRangeCriteria(


(sugg, nb): it seems like the logic here is almost the same as the standard one - would it make sense to combine and instead branch at the point of formatting the primary clause?

mcmcgrath13 · 2026-05-11T13:30:20Z

    assertThat(mapped.reportColumnUid()).isEqualTo(column.getId());
-    assertThat(mapped.defaultValue()).isEqualTo(List.of("value"));
+    assertThat(mapped.defaultValues()).isEqualTo(List.of("value"));
+    assertThat(mapped.minValueCount()).isEqualTo(1);


(sugg, b): can we add the defaultIncludeNulls to the assertion block?

(and have a test where it will be false)

added for true and false

mcmcgrath13 · 2026-05-11T13:32:49Z

+  @BeforeEach
+  void setUp() {
+    // Instantiate real dependencies
+    FieldFormatter fieldFormatter = new FieldFormatter();


mcmcgrath13 · 2026-05-11T13:35:16Z

+      Long reportColumnUid,
+      Boolean defaultIncludeNulls,
+      String type) {
+    BasicFilterConfiguration basicFilterConfiguration =


(q, nb): why mock here vs use the real filter configuration? it's a relatively straightforward object

got a little mock happy on the work. Switched to instantiated config

mcmcgrath13 · 2026-05-11T13:45:00Z

+    String whereFragment =
+        mockWhereClauseService.buildBasicWhereFragment(reportConfig, basicFilterRequest);
+
+    assertThat(whereFragment).isEqualTo("([ColumnName] IN ('condition1') OR [ColumnName] IS NULL)");


(sugg, b): could we add a test that combines this with the AND-ing to ensure overall precedence/parens end up correct?

good call - added

sonarqubecloud · 2026-05-12T16:20:40Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

emyl3 and others added 10 commits April 13, 2026 16:50

wip

dc6856e

feature: updates FilterType field names and change from foreach to fo…

f57a96a

…r loop

feature: continer and append closing parenthesis

2278074

db: where clause rough draft

a669239

feature: add tests and build where clause service

8e93e4d

Merge branch 'main' into el/app-367

de152d9

# Conflicts: # apps/modernization-api/src/main/java/gov/cdc/nbs/report/ReportService.java

test: write WhereClauseServiceTest first test

1c4470d

test: add more tests for WhereClauseService

248fbd6

Merge branch 'main' into el/app-367

5027aad

test: add tests for FieldFormatter

d7b08e8

brick-green changed the title ~~VERY WIP Handoff - [APP-367] feat: Update subset_sql to use basic filters from request~~ APP-367] feat: Update subset_sql to use basic filters from request Apr 22, 2026

brick-green changed the title ~~APP-367] feat: Update subset_sql to use basic filters from request~~ [APP-367] feat: Update subset_sql to use basic filters from request Apr 22, 2026

ui-update: auto update UI fields

4cab398

brick-green marked this pull request as ready for review April 22, 2026 15:15

brick-green requested a review from a team as a code owner April 22, 2026 15:15

brick-green requested review from JordanGuinn and mcmcgrath13 and removed request for a team April 22, 2026 15:15

mcmcgrath13 reviewed Apr 22, 2026

View reviewed changes

Comment thread apps/modernization-api/src/main/java/gov/cdc/nbs/report/WhereClauseService.java

mcmcgrath13 reviewed Apr 22, 2026

View reviewed changes

Comment thread apps/modernization-api/src/main/java/gov/cdc/nbs/report/WhereClauseService.java Outdated

mcmcgrath13 reviewed Apr 22, 2026

View reviewed changes

Comment thread apps/modernization-api/src/main/java/gov/cdc/nbs/report/WhereClauseService.java Outdated

mcmcgrath13 requested changes Apr 22, 2026

View reviewed changes

brick-green marked this pull request as draft April 23, 2026 16:43

brick-green added 4 commits April 23, 2026 14:24

feature: setup nullable flags and fix tests

5c2a927

add additional tests

40e9250

npm run generate

ebd9bd9

mcmcgrath13 reviewed May 6, 2026

View reviewed changes

brick-green added 6 commits May 7, 2026 17:09

review: post review updates

90a1327

update from npm run generate

0e482a5

account for issue with formattedValues missing

783ceed

typo

ed48055

Merge branch 'main' into el/app-367

2ab19ed

remove unneeded test

f6282c9

brick-green requested review from JordanGuinn and mcmcgrath13 May 8, 2026 14:38

mcmcgrath13 reviewed May 11, 2026

View reviewed changes

brick-green added 7 commits May 11, 2026 11:10

front-end: property names to align with backend

477d343

prettier format

ef05eae

add tests and convert WHERE to constant for equality checking

b87d77c

build BasicFilterConfiguration as much as possible instead of mocking

2580957

spotless

9248f40

Merge branch 'refs/heads/main' into el/app-367

a775c04

fix birth years test

95bddb4

brick-green requested a review from mcmcgrath13 May 12, 2026 17:44

	List<String> yearRange = List.of("2023", "2023");
	List<String> yearRange = List.of("2023", "2024");

	assertThat(whereFragment).isEqualTo("([ColumnName] IN (condition1))");
	assertThat(whereFragment).isEqualTo("([ColumnName] IN ('condition1'))");

Conversation

emyl3 commented Apr 13, 2026 • edited by brick-green Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tickets

Checklist before requesting a review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcmcgrath13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmcgrath13 May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 12, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emyl3 commented Apr 13, 2026 •

edited by brick-green

Loading

mcmcgrath13 May 11, 2026 •

edited

Loading