Skip to content

Correct query to ignore aliases. #1672

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
MikeNeilson merged 2 commits into from
Apr 7, 2026
+48 −15
Merged

Correct query to ignore aliases. #1672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e42a358
Correct query.
MikeNeilson Apr 3, 2026
c34d6fb
Include aliases within single TimeSeriesDescriptor retrieval.
MikeNeilson Apr 6, 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
37 changes: 25 additions & 12 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/TimeSeriesIdentifierDescriptorDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,19 +224,32 @@ private TimeSeriesIdentifierDescriptor toDescriptorWithAliases(Record6<String, S

public Optional<TimeSeriesIdentifierDescriptor> getTimeSeriesIdentifier(String office, String timeseriesId) {
AV_CWMS_TS_ID2 view = AV_CWMS_TS_ID2.AV_CWMS_TS_ID2;
return connectionResult(dsl, connection -> {
Record5<String, String, Long, String, String>
result = dsl.select(view.CWMS_TS_ID, view.DB_OFFICE_ID, view.INTERVAL,
view.TIME_ZONE_ID, view.TS_ACTIVE_FLAG)
Table<?> innerTable = AV_CWMS_TS_ID2.AV_CWMS_TS_ID2.as("alias_table");
Field<String> tsId = innerTable.field("CWMS_TS_ID", String.class);
Field<BigDecimal> innerTsCode = innerTable.field("TS_CODE", BigDecimal.class);
Field<String> aliasedItem = innerTable.field("ALIASED_ITEM", String.class);
return dsl
.select(view.DB_OFFICE_ID,
view.CWMS_TS_ID,
view.INTERVAL_UTC_OFFSET,
view.TS_ACTIVE_FLAG,
view.TIME_ZONE_ID,
DSL.multiset(
dsl.selectDistinct(
tsId
).from(innerTable)
.where(view.TS_CODE.eq(innerTsCode))
.and(aliasedItem.isNotNull())
).convertFrom(rs -> rs.map(r -> r.get(tsId, String.class)))
)
.from(view)
.where(view.CWMS_TS_ID.eq(timeseriesId).and(view.DB_OFFICE_ID.eq(office.toUpperCase()))).fetchOne();
Optional<TimeSeriesIdentifierDescriptor> retval = Optional.empty();
if (result != null) {
retval = Optional.of(toDto(result));
}

return retval;
});
.where(view.CWMS_TS_ID.eq(timeseriesId)
.and(view.DB_OFFICE_ID.eq(office.toUpperCase())))
.orderBy(view.DB_OFFICE_ID, view.CWMS_TS_ID,
view.INTERVAL_UTC_OFFSET,
view.TS_ACTIVE_FLAG,
view.TIME_ZONE_ID)
.fetchOptional(this::toDescriptorWithAliases);
Copy link
Copy Markdown
Contributor Author

@MikeNeilson MikeNeilson Apr 6, 2026

Choose a reason for hiding this comment

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

So I copied this from the getAll handler above, and initially left it at .stream().findFirst(). Now Idea why but in that usage the underlying connection never closed so the fetchOptional was used and that error went away.

Not a huge deal, but figured I'd mention it for others to be aware of.

Copy link
Copy Markdown
Collaborator

@adamkorynta adamkorynta Apr 6, 2026

Choose a reason for hiding this comment

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

I believe Ryan has run into this before and did an audit previously to check for that kind of usage. The jOOQ documentation is misleading: https://www.jooq.org/doc/latest/manual/sql-execution/fetching/lazy-fetching-with-streams/ - in that second example, it makes it look like you don't have to handle the resources unlike the first example.

Lukas has commented on: https://stackoverflow.com/questions/35558364/do-i-risk-a-jdbc-connection-leak-when-streaming-jooq-results-outside-a-try-with which is the reference we had looked at before to determine where the leak was coming from.

Copy link
Copy Markdown
Contributor Author

@MikeNeilson MikeNeilson Apr 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That would seem to imply the queries I copied are incorrect. And yet they seem to pass the test just fine. Checking that was part of why I set it to a repeated test.

Copy link
Copy Markdown
Collaborator

@rma-rripken rma-rripken Apr 6, 2026

Choose a reason for hiding this comment

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

I have previously double-checked all our usages of stream wrt to jooq. I believe the query you copied from was modified in Dec. It may have been missed in review. Stream() works and its super natural to use and it looks clean but to work correctly with jooq I believe it must be inside a try-with-resources block. My understanding is that jooq's stream() call also pulls the entire result set before streaming it. Since you need to wrap the block in the try-with-resources anyway seems likr you might as well use fetchLazy and get whatever minor performance improvements you can.
I skimmed the jooq implementation of fetch and it looked to me like it was doing try-with-resources around fetchLazy with an internal call to collect so I'd suggest replacing any usages of the pattern of calling stream().map(...).collect(...) with .fetch(Mapper).

I'm not sure why your findFirst wouldn't close the connection but if you wanted to find out I'd set a breakpoint right before that jooq call and then when you are at that break-point I'd add/enable a new generic breakpoint on any thrown exceptions and see what pops up. It sounds like fetchOptional is what you want anyways so if you believe my previous arg against using stream() then fetchOptional is the right thing anyways and its not worth investigating.

Copy link
Copy Markdown
Contributor Author

@MikeNeilson MikeNeilson Apr 6, 2026

Choose a reason for hiding this comment

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

Thanks. I'll just leave it for now, as all the tests pass. I'm I'm curious enough that I'll investigate later in the week so we can know for sure.

}

public static TimeSeriesIdentifierDescriptor toDto(Record5<String, String, Long, String, String> rec) {
Expand Down
26 changes: 23 additions & 3 deletions cwms-data-api/src/test/java/cwms/cda/api/TimeSeriesIdentifierDescriptorControllerTestIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.RepeatedTest;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
Expand Down Expand Up @@ -530,7 +531,7 @@ void pagingTest() throws Exception {
}


@Test
@RepeatedTest(2)
void testAliasedTimeSeries() throws Exception {
// Test Structure
//
Expand All @@ -541,7 +542,8 @@ void testAliasedTimeSeries() throws Exception {
// 5. Retrieve with aliases

createLocation("Alder Springs", true, "SPK");
String likePattern = "Alder Springs\\.Precip-Cumulative\\.Inst\\.15Minutes\\.0\\.DescriptorTEST_ALIASES.*";
final String likePattern = "Alder Springs\\.Precip-Cumulative\\.Inst\\.15Minutes\\.0\\.DescriptorTEST_ALIASES.*";
final String formatPattern = "Alder Springs.Precip-Cumulative.Inst.15Minutes.0.DescriptorTEST_ALIASES%d";

// Check that we don't have any ts like this in the catalog.
List<String> names = getIdsLike(OFFICE, likePattern);
Expand All @@ -553,7 +555,7 @@ void testAliasedTimeSeries() throws Exception {
// Create a bunch of ts and store them.
int count = 8;
for (int i = 0; i < count; i++) {
String tsId = String.format("Alder Springs.Precip-Cumulative.Inst.15Minutes.0.DescriptorTEST_ALIASES%d", i);
String tsId = String.format(formatPattern, i);
TimeSeriesIdentifierDescriptor ts = buildTimeSeriesIdentifierDescriptor(OFFICE, tsId);
String serializedTs = om.writeValueAsString(ts);

Expand Down Expand Up @@ -668,6 +670,24 @@ void testAliasedTimeSeries() throws Exception {
.assertThat()
.statusCode(is(HttpServletResponse.SC_OK))
.body("descriptors[0].aliases.size()", greaterThan(0));


// get by path single
given()
.log().ifValidationFails(LogDetail.ALL, true)
.accept(Formats.JSONV2)
.queryParam(Controllers.OFFICE, OFFICE)
.queryParam(Controllers.TIMESERIES_ID_REGEX, likePattern)
.queryParam(Controllers.EXCLUDE_EMPTY, false)
.queryParam(Controllers.INCLUDE_ALIASES, true)
.when()
.get("/timeseries/identifier-descriptor/{timeseries-id}", String.format("Alder Springs.Precip-Cumulative.Inst.15Minutes.0.DescriptorTEST_ALIASES%d",0))
.then()
.log().ifValidationFails(LogDetail.ALL, true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_OK))
.body("aliases.size()", greaterThan(0));

}

@Test
Expand Down
Loading