[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-04T15:03:35Z

+
+    StringBuilder segment = new StringBuilder("(");
+    String colName = "[" + column.columnName() + "]"; // Brackets protect against SQL reserved words
+    boolean hasValues = !formattedValues.isEmpty();


(q, nb): when could this be empty given the input values is known to not be empty?

Maybe worth throwing if there are somehow no formatted values? Or if the number of formatted values somehow doesn't match the number of values on the request (which should never happen, but yannow)

throwing an exception here now for both the scenarios

JordanGuinn · 2026-05-06T20:51:04Z

+    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): We could also check to see if basicWherefragment returns a non-empty value? Not a huge deal by any means, just seems a bit precarious to check against the base WHERE clause length.

my thought was a buildAdvancedWhereFragment() would be added next so that check on the WHERE length would cover for both. What do you think?

Ahhh, sure that makes sense. I suppose so long as we have tests covering both those cases, and the resulting where clause output, that should be fine!

JordanGuinn · 2026-05-06T20:51:39Z

(nit, nb): Thoughts on naming this WhereClauseBuilder or similar, in the spirit of following the pattern of ReportSpecBuilder?

JordanGuinn · 2026-05-06T20:55:08Z

+    } catch (DateTimeParseException e) {
+      throw new IllegalArgumentException("Can't Convert Date: " + date);
+    }
+    // Produces '2023-12-31'


(q, nb): Is this true? It's not the output I would expect with us parsing to "MM/dd/yyyy"

That's the default SQL Server date format. So it seemed like the best format to converge on for building the SQL criteria

JordanGuinn · 2026-05-06T21:05:08Z

+
+    StringBuilder segment = new StringBuilder("(");
+    String colName = "[" + column.columnName() + "]"; // Brackets protect against SQL reserved words
+    boolean hasValues = !formattedValues.isEmpty();


Maybe worth throwing if there are somehow no formatted values? Or if the number of formatted values somehow doesn't match the number of values on the request (which should never happen, but yannow)

JordanGuinn · 2026-05-06T21:07:50Z

+
+    // Malicious Input: Attempt to close the IN clause and drop the Reports table
+    // The -- at the end comments out the rest of your generated query (the closing parentheses)
+    String maliciousInput = "'); DROP TABLE Reports; --";


JordanGuinn · 2026-05-06T21:09:19Z

@@ -30,8 +31,15 @@ public static BasicFilterConfiguration fromReportFilter(ReportFilter filter) {
    // For the future: A list of strings may end up being too simple for all use cases,
    // may need to evolve to be a small object with a key and value
    List<String> defaultValue = null;


Suggested change

List<String> defaultValue = null;

List<String> defaultValues = null;

mcmcgrath13 · 2026-05-06T22:34:34Z

    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) Boolean isRequired,
-    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) FilterType filterType) {}
+    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) FilterType filterType,
+    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) Boolean defaultIncludeNulls) {}


(nit, nb): the minorest of baby nits, but I'd probably put this field just after defaultValues for colocation of sort of thing reasons

mcmcgrath13 · 2026-05-06T22:51:54Z

+
+    StringBuilder criteria = new StringBuilder("(");
+
+    String colName = "[" + column.columnName() + "]";


(sugg, nb): probably overkill, but do we want a utility somewhere for escaping/formatting column Names? We do it here and elsewhere in the query building

mcmcgrath13 · 2026-05-06T22:55:22Z

+    assertThat(formatter.formatField("INTEGER", "123")).isEqualTo("123");
+    assertThat(formatter.formatField("NUMBER", "45.67")).isEqualTo("45.67");


(sugg, b): for security, can we ensure that the numbers are number shaped before allowing them through?

good call! Added

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?

sonarqubecloud · 2026-05-08T14:46:02Z

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 4, 2026

View reviewed changes

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

brick-green added 9 commits May 5, 2026 12:31

feature: big update for tests and PR comments

e84e6bb

fix report spec builder test

53f8a53

Merge branch 'main' into el/app-367

4afaa48

sonarqube rules

075df8e

Merge branch 'main' into el/app-367

a866849

spotless

ab5d7f8

npm run generate

2345a10

Merge branch 'main' into el/app-367

55758b8

npm run generate again

3e3069b

brick-green requested a review from mcmcgrath13 May 6, 2026 19:12

JordanGuinn reviewed May 6, 2026

View reviewed changes

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

	List<String> defaultValue = null;
	List<String> defaultValues = null;


		StringBuilder criteria = new StringBuilder("(");

		String colName = "[" + column.columnName() + "]";

		assertThat(formatter.formatField("INTEGER", "123")).isEqualTo("123");
		assertThat(formatter.formatField("NUMBER", "45.67")).isEqualTo("45.67");

	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!

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!

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 8, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

emyl3 commented Apr 13, 2026 •

edited by brick-green

Loading