Skip to content

Conversation

@labkey-chrisj
Copy link
Contributor

Rationale

This adds test coverage for Issue 52739
Related github issue https://github.com/LabKey/kanban/issues/949

Related Pull Requests

n/a

Changes

New test method in PivotQueryTest.java

Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug was in how we handled the SQL for an invalid pivot query. The query here looks like it's been fixed and is running successfully.

To ensure that we cover the original issue, please remove F1 from the SELECT list and ensure that you get a reasonable error message instead of a NullPointerException. It's fine to also check the corrected version of the query as you're doing here.

Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. This looks fine. FWIW, uou could also have used the executeSql API which would be a little more efficient.

public void testBadPivotQuery()
{
String datasetName = TestDataGenerator.randomDomainName("D2", DomainUtils.DomainKind.StudyDatasetVisit);
String textFieldName = TestDataGenerator.randomFieldName("F1", ":,;'\"");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason you are excluding these :,;'\" characters?
TestDataGenerator.randomFieldName will select a valid field name for a given domain:
public static String randomFieldName(@NotNull String part, @Nullable Integer numStartChars, @Nullable Integer numEndChars, @Nullable String exclusion, @Nullable DomainUtils.DomainKind domainKind)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used those in an earlier test like this one, probably shouldn't here. I'll pull those out.

@labkey-chrisj labkey-chrisj merged commit 5d731fc into develop Aug 4, 2025
6 checks passed
@labkey-chrisj labkey-chrisj deleted the fb_issue52739_test branch August 4, 2025 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants