Add support for virtual location levels #1086

zack-rma · 2025-04-28T21:42:15Z

Fixes #469

Adds support for virtual location levels to the levels endpoint. Integration testing included.

MikeNeilson · 2025-04-29T13:37:18Z

@zack-rma merge develop in for the nicer test report.

MikeNeilson

Generally looks reasonable. Only issue is with the level of duplication within the test code.

MikeNeilson · 2025-05-01T21:06:59Z

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

+        assertThat(response.path("constituents[1].name"), equalTo(existingSpec));
+        assertThat(response.path("constituents[1].type"), equalTo("RATING"));
+
+        CwmsDataApiSetupCallback.getDatabaseLink().connection(c -> {


These won't get called on failure which can hinder repeatability of tests.

If the location is registered with create location the cascade delete should take care of the ratings so these shouldn't be required.

Removed ratings cleanup methods

MikeNeilson · 2025-05-01T21:08:09Z

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

+        String specXml = RatingSpecXmlFactory.toXml(specContainer, "", 0, true);
+        String setXml = RatingContainerXmlFactory.toXml(container, "", 0, true, false);
+
+        CwmsDataApiSetupCallback.getDatabaseLink().connection(c -> {


This large block is repeated in a near identical way. Some sort of helper function (similar to createLocation) should be created.

Added helper methods to reduce code duplication

MikeNeilson · 2025-05-02T20:37:49Z

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

@@ -808,4 +1266,63 @@ enum GetAllTestNewAliases
            _expectedContentType = expectedContentType;
        }
    }
+
+    private String createRatingSpec(String locationName) throws Exception


Preferably these would've gone in the DataApiTest class, but this at least makes it easier for the next person to yank up and make more generic.

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

cwms-data-api/src/main/java/cwms/cda/data/dto/VirtualLocationLevel.java

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

cwms-data-api/src/main/java/cwms/cda/data/dto/VirtualLocationLevel.java

…el subtypes

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

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/SeasonalLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/TimeSeriesLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/ConstantLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

cwms-data-api/src/test/resources/cwms/cda/api/virtuallevels/virtual_level_3.json

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

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/VirtualLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

zack-rma

Some issues resolved, more fixes to come

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

cwms-data-api/src/test/resources/cwms/cda/api/virtuallevels/virtual_level_3.json

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/TimeSeriesLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dao/LocationLevelsDaoImpl.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/ConstantLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/VirtualLocationLevel.java

MikeNeilson · 2025-05-06T21:51:16Z

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

+@FormattableWith(contentType = Formats.JSONV1, formatter = JsonV1.class)
+@FormattableWith(contentType = Formats.XMLV2, formatter = XMLv2.class, aliases = {Formats.XML})
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class LocationLevel extends CwmsDTO


@adamkorynta considering this new class hierarchy can we ever meaningfully have a plain "LocationLevel"? if not seems like this should be marked abstract.

Made abstract

MikeNeilson · 2025-05-06T21:55:27Z

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevels.java

@@ -15,7 +42,8 @@

 @JsonRootName("location-levels")
 @FormattableWith(contentType = Formats.JSONV2, formatter = JsonV2.class, aliases = {Formats.DEFAULT, Formats.JSON})
-public class LocationLevels extends CwmsDTOPaginated {
+public class LocationLevels extends CwmsDTOPaginated


we do keep the curly on the same line in this project.

MikeNeilson · 2025-05-08T21:08:50Z

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

                }
+            } catch (NullPointerException e) {
+                //gets thrown if required field is null
+                throw new IllegalArgumentException(e.getMessage());


we should include the exception so that the stacktrace can hit the log and people can trace issues, should they happen.

also what is throwing null that we can't check for directly?

Updated throw to include the exception.

Looking into the NPE catch now.

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/ConstantLocationLevel.java

MikeNeilson · 2025-05-08T21:20:10Z

cwms-data-api/src/test/java/cwms/cda/data/dto/LocationLevelTest.java


 		ContentType contentType = Formats.parseHeader(Formats.JSONV2, LocationLevel.class);
 		String jsonStr = Formats.format(contentType, level);

-		// If JSONv2 isn't setup correctly it will serialize the level like:


even though these comments are duplicate, they should be retained, and probably even added to the one above.

anyone of these tests should be reasonable in isolation so it is reasonable to see duplication we would otherwise avoid in normal code.

Added comments to other similar methods.

…ng, removed VirtualLocationLevel XML support

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

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/ConstantLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/VirtualLocationLevel.java

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

MikeNeilson

Some nitpicks, and an change to the builder pattern.

MikeNeilson · 2025-05-20T14:46:55Z

cwms-data-api/src/main/java/cwms/cda/data/dao/LocationLevelsDaoImpl.java

-        int newPageSize = pageSize - builderMap.size();
-        int newOffset = offset - builderMap.size(); // TODO: check if this is correct, this may cause issues with paging
+        int virtualPageSize = pageSize - builderMap.size();
+        int virtualOffset = offset - builderMap.size(); // TODO: check if this is correct, this may cause issues with paging


this todo seems fairly important.

Updated query to combine tables for consistent paging results. In order to get the VirtualLocationLevel constituents list a subquery would be needed, along with some more logic that is currently in a PL/SQL method for a single retrieval. Retrieving a single VirtualLocationLevel would include the constituent data, currently.

MikeNeilson · 2025-05-20T14:48:16Z

cwms-data-api/src/main/java/cwms/cda/data/dao/LocationLevelsDaoImpl.java

@@ -574,49 +569,44 @@ private VirtualLocationLevel buildVirtualLocationLevel(LOCATION_LEVEL_T level, S

    private ConstantLocationLevel buildConstantLocationLevel(LOCATION_LEVEL_T level, String officeId, String units,
            String locationLevelName, ZonedDateTime realEffectiveDate, Double constantValue) {
-        return new ConstantLocationLevel.Builder(locationLevelName, realEffectiveDate)
+        return ((ConstantLocationLevel.Builder) new ConstantLocationLevel.Builder(locationLevelName, realEffectiveDate)


This cast seems odd.

Removed unnecessary casts.

MikeNeilson · 2025-05-20T14:51:42Z

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/SeasonalLocationLevel.java

+		} else {
+			throw new RequiredFieldException("Either interval-minutes or interval-months must be set, but not both");
+		}
+		validator.mutuallyExclusive("Only one of the following can be defined at once for a seasonal location level: "


this duplicates the login of the above if/else if/else section.

Removed duplicate logic above

MikeNeilson · 2025-05-20T14:54:55Z

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

@@ -155,10 +155,10 @@ void test_retrieve_effective_date() throws Exception {
        String levelId = "level_with_effect.Flow.Ave.1Day.Regulating";
        ZonedDateTime time = ZonedDateTime.of(2023, 6, 1, 0, 0, 0, 0, ZoneId.of("America/Los_Angeles"));
        CwmsDataApiSetupCallback.getDatabaseLink().connection(c -> {
-            LocationLevel level = new ConstantLocationLevel.Builder(levelId, time)
+            LocationLevel level = ((ConstantLocationLevel.Builder) new ConstantLocationLevel.Builder(levelId, time)


Hmm, the amount of casting you're doing really shouldn't be required.

Was it the compiler or the IDE complaining about this? Because somehow this behaves right for the things we do in testcontainers-cwms.. I'll go back and look at the root location level builder.

Removed unnecessary casts

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

…constituents

zack-rma · 2025-05-23T17:08:19Z

@MikeNeilson It seems this PR and #1109 are failing from the StreamDAOTestIT failing in the same way. It seems to be inconsistent, as it hasn't been failing on all PRs...

MikeNeilson

My concerns have been address.

…virtual_loc_levels

adamkorynta · 2025-06-06T19:42:42Z

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

+        }
+    }
+
+    public static <T extends LocationLevel> T castLocationLevel(LocationLevel level, ZonedDateTime unmarshalledDateTime) {


This is not doing what you think it is doing...

Removed method and refactored logic

adamkorynta · 2025-06-06T19:43:04Z

cwms-data-api/src/main/java/cwms/cda/data/dto/locationlevel/LocationLevel.java

+            attributeUnitsId = null;
+        }
+
+        if (existingLevel instanceof VirtualLocationLevel) {


All of the common builder attributes should get extracted

Extracted common builder attributes

zack-rma added 5 commits April 30, 2025 13:33

Integration testing in progress

c764bfd

Updated constituent handling, tests in progress

b846e8d

Added delete method support

b7f503e

Deletion testing in progress

5086782

Deletion testing completed

09fb5be

zack-rma force-pushed the feature/virtual_loc_levels branch from c4b372b to 09fb5be Compare April 30, 2025 20:34

zack-rma marked this pull request as ready for review May 1, 2025 15:43

zack-rma requested review from adamkorynta and MikeNeilson May 1, 2025 15:43

MikeNeilson requested changes May 1, 2025

View reviewed changes

zack-rma added 2 commits May 1, 2025 14:57

Cleaned up testing, added creation helper functions

f9c4e69

Removed ratings cleanup

840bb31

zack-rma requested a review from MikeNeilson May 1, 2025 22:02

MikeNeilson previously approved these changes May 2, 2025

View reviewed changes

adamkorynta reviewed May 5, 2025

View reviewed changes

cwms-data-api/src/main/java/cwms/cda/api/LevelsController.java Outdated Show resolved Hide resolved

adamkorynta requested changes May 5, 2025

View reviewed changes

adamkorynta reviewed May 5, 2025

View reviewed changes

cwms-data-api/src/main/java/cwms/cda/data/dto/VirtualLocationLevel.java Outdated Show resolved Hide resolved

Refactored LocationLevel, added child classes. Updated controller, DAO

54ff757

zack-rma dismissed MikeNeilson’s stale review via 54ff757 May 6, 2025 21:41

Moved interpolate parameter, updated controller to handle LocationLev…

912c138

…el subtypes

zack-rma requested a review from adamkorynta May 6, 2025 22:48

adamkorynta linked an issue May 7, 2025 that may be closed by this pull request

Add support for virtual location levels #469

Open

adamkorynta requested changes May 7, 2025

View reviewed changes

Updated per feedback, added subclasses

392e35c

zack-rma commented May 8, 2025

View reviewed changes

MikeNeilson reviewed May 8, 2025

View reviewed changes

zack-rma added 3 commits May 9, 2025 09:13

Updates from feedback. Fixed scoping and builders

fe6211e

Updates from feedback. Removed unnecessary query param, updated loggi…

6ef2488

…ng, removed VirtualLocationLevel XML support

Formatting fixes

cc3e071

Minor unit test fixes

24c67cb

zack-rma requested review from adamkorynta and rma-psmorris May 15, 2025 17:15

adamkorynta requested changes May 19, 2025

View reviewed changes

MikeNeilson requested changes May 20, 2025

View reviewed changes

zack-rma added 4 commits May 20, 2025 16:05

Updated naming, generic typing

321df49

Update query to handle paging. Catalog will not return virtual level …

3d211bd

…constituents

Paging test update

76105c6

Improved paging test coverage

fffb3d5

MikeNeilson previously approved these changes May 27, 2025

View reviewed changes

Updated virtual location level test data

451c511

zack-rma dismissed MikeNeilson’s stale review via 451c511 June 5, 2025 18:35

zack-rma requested a review from adamkorynta June 5, 2025 18:36

zack-rma added 3 commits June 6, 2025 11:24

Merge remote-tracking branch 'refs/remotes/hec/develop' into feature/…

c0475c6

…virtual_loc_levels

Fix broken symbol reference

62659ab

Fix broken latest rating test

62e8d55

adamkorynta reviewed Jun 6, 2025

View reviewed changes

zack-rma added 2 commits June 6, 2025 12:55

Removed unnecessary casting method

f70c4e0

extracted common builder attributes

cdd814f

Add support for virtual location levels #1086

Are you sure you want to change the base?

Add support for virtual location levels #1086

Conversation

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

Uh oh!

Uh oh!

MikeNeilson commented Apr 29, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zack-rma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MikeNeilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zack-rma commented Apr 28, 2025 •

edited

Loading