CWMS-2024: New LRTS identifier support #1126

zack-rma · 2025-06-09T18:32:21Z

CWMS-2024: New LRTS identifier support through header and integration testing

MikeNeilson · 2025-06-09T19:45:24Z

cwms-data-api/src/main/java/cwms/cda/ApiServlet.java

@@ -267,6 +265,7 @@ public class ApiServlet extends HttpServlet {
    public static final String DATA_SOURCE = "data_source";
    public static final String RAW_DATA_SOURCE = "data_source";
    public static final String DATABASE = "database";
+    public static final String IS_NEW_LRTS = "X-NEW_LRTS";


X-CWMS-LRTS-Formatting

or something like that, but at a minimum: X-CWMS-...

I'd argue that we don't want LRTS as we're talking about the interval, so something like X-CWMS-Use-LocalRegularInterval. But then again maybe client usage will only be thinking in context of time series so LRTS is clearer. Thoughts?

MikeNeilson · 2025-06-10T15:16:46Z

cwms-data-api/src/test/java/cwms/cda/api/TimeSeriesIdentifierDescriptorControllerTestIT.java

+            .when()
+                .redirects().follow(true)
+                .redirects().max(3)
+                .delete("/timeseries/identifier-descriptor/" + tsId)


Suggested change

.delete("/timeseries/identifier-descriptor/" + tsId)

.delete("/timeseries/identifier-descriptor/{tsID}", tsId)

There's a minor improvement in the log output when using this form. Also makes it easier to look at when there's more than one parameter.

Adjusted path format as suggested.

MikeNeilson · 2025-06-10T15:17:37Z

cwms-data-api/src/test/java/cwms/cda/api/rating/RatingsControllerTestIT.java

@@ -68,7 +68,7 @@ static void beforeAll() throws Exception
 		String ratingXml = readResourceFile("cwms/cda/api/Zanesville_Stage_Flow_COE_Production.xml");
 		ratingXml = ratingXml.replaceAll("Zanesville", EXISTING_LOC);
 		String ratingXml2 = ratingXml.replaceAll("2002-04-09T13:53:01Z", "2016-06-06T00:00:00Z");
-		String ratingXml3 = ratingXml.replaceAll("2002-04-09T13:53:01Z", "2025-06-06T00:00:00Z");
+		String ratingXml3 = ratingXml.replaceAll("2002-04-09T13:53:01Z", "2065-06-06T00:00:00Z");


Let's put this in a separate PR. You're already going to have to update several otherwise complete branches.... and I have one that needs it as well.

#1149 Created a PR here

zack-rma · 2025-06-12T17:27:13Z

The current endpoints that return the legacy LRTS interval identifier despite the database being set to use the new format are:

BinaryTimeSeries - GET
Levels - GET
TimeSeriesGroups - GET

This will be updated if more problematic endpoints are discovered.

An issue has been created for cwms-database, and is visible here: HydrologicEngineeringCenter/cwms-database#24

adamkorynta

Integration tests should test both new and old format and assert that the returned identifier/name matches the request old/new format.

If the client passes in the new identifier without the requisite header, that should be a client error 400, not an internal server error 500.

adamkorynta · 2025-06-13T15:43:19Z

cwms-data-api/src/test/java/cwms/cda/api/BinaryTimeSeriesControllerTestIT.java

+        .then()
+            .log().ifValidationFails(LogDetail.ALL,true)
+        .assertThat()
+            .statusCode(is(HttpServletResponse.SC_INTERNAL_SERVER_ERROR));


Should be a client error

Added error checking and regex matching, now throws CONFLICT client error.

adamkorynta · 2025-06-13T15:44:19Z

cwms-data-api/src/test/java/cwms/cda/api/BinaryTimeSeriesControllerTestIT.java

+
+        // Structure of test:
+        //
+        // 1)Try to create the binary time series without LRTS


Suggested change

// 1)Try to create the binary time series without LRTS

// 1)Try to create the binary time series using new LRTS format without the new LRTS format header

Updated comment as suggested

adamkorynta · 2025-06-13T15:45:35Z

cwms-data-api/src/test/java/cwms/cda/api/BinaryTimeSeriesControllerTestIT.java

+            .queryParam(Controllers.NAME, tsIdentifier)
+            .queryParam(Controllers.BEGIN, "2004-05-01T12:00:00Z")
+            .queryParam(Controllers.END, "2007-05-19T16:00:00Z")
+            .header(ApiServlet.IS_NEW_LRTS, true)


Add a test right after this that tests without the new header to assert that the old interval id is returned

Added test to confirm old interval ID is returned

adamkorynta · 2025-06-13T15:46:37Z

cwms-data-api/src/test/java/cwms/cda/api/BinaryTimeSeriesControllerTestIT.java

+            .log().ifValidationFails(LogDetail.ALL,true)
+        .assertThat()
+            .statusCode(is(HttpServletResponse.SC_OK))
+            .body("binary-values.size()", equalTo(3))


Need to test that the new identifier is returned to the client as part of the name.

Added check for name value

adamkorynta · 2025-06-13T15:51:25Z

cwms-data-api/src/test/java/cwms/cda/api/DataApiTestIT.java

+            org.jooq.Configuration configuration = DSL.using(c).configuration();
+            CWMS_UTIL_PACKAGE.call_SET_SESSION_INFO(configuration,
+                SESSION_USE_LRTS_ID_FORMAT, requireBool, requireIntValue);
+            try(PreparedStatement stmt = c.prepareStatement(createTimeseriesQuery)) {


This query does not distinguish between PRTS and LRTS using the old identifier

Refactored method to only be used for new interval identifier. Should an alternative query specifying LRTS vs PRTS be used here?

…upport # Conflicts: # cwms-data-api/src/test/java/cwms/cda/api/TimeseriesControllerTestIT.java

New LRTS identifier header support and initial testing

f4e2ce1

MikeNeilson reviewed Jun 9, 2025

View reviewed changes

zack-rma added 4 commits June 9, 2025 13:22

Added further test coverage, updated header parameter name

81b34cc

Added further test coverage, testing in progress

6cce6f2

Forecast tests fixed

3e869da

Tests updated

f4160a2

MikeNeilson reviewed Jun 10, 2025

View reviewed changes

zack-rma added 4 commits June 10, 2025 11:34

Merge branch 'develop' into feature/lrts-header-support

a89f78c

Updates to test cases and test data

f9c9cb9

Updated tests

37a7a23

Merge branch 'develop' into feature/lrts-header-support

66004d4

Update method tests

88110e2

adamkorynta requested changes Jun 13, 2025

View reviewed changes

zack-rma added 5 commits June 17, 2025 11:23

Removed legacy TS IDs from tests

5e5b712

Updated binary TS tests, error handling and regex matching

4c14f28

Disabled broken tests, fixed legacy id test for ts controller

f7467f5

Merge remote-tracking branch 'hec/develop' into feature/lrts-header-s…

68a130b

…upport # Conflicts: # cwms-data-api/src/test/java/cwms/cda/api/TimeseriesControllerTestIT.java

Updated binary test, fixed merge conflict

0c6410a

zack-rma marked this pull request as ready for review June 19, 2025 21:03

Updated timeseries creation method for new LRTS interval identifier

b93707d

zack-rma requested a review from adamkorynta June 19, 2025 23:30

Added assertions for TS ids, disabled failing test

698af2b

	.delete("/timeseries/identifier-descriptor/" + tsId)
	.delete("/timeseries/identifier-descriptor/{tsID}", tsId)

	// 1)Try to create the binary time series without LRTS
	// 1)Try to create the binary time series using new LRTS format without the new LRTS format header

CWMS-2024: New LRTS identifier support #1126

Are you sure you want to change the base?

CWMS-2024: New LRTS identifier support #1126

Uh oh!

Conversation

zack-rma commented Jun 9, 2025

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!

zack-rma commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamkorynta 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!

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!

zack-rma commented Jun 12, 2025 •

edited

Loading