Fixes #634 - Implemented data entry date option for TS data retrieval #927

zack-rma · 2024-10-22T16:13:38Z

Fixes #634 - Implements data entry date as option for TimeSeries data retrieval - Serialization bug in progress

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

MikeNeilson · 2024-10-22T16:38:02Z

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

@@ -248,6 +256,11 @@ public static class Record {
        @JsonProperty(value = "quality-code", index = 2)
        int qualityCode;



This might be better as a sub class. I know that adds some complex but eventually we may also want to include the version date and text notes that may be attached and that would be a lot of logic in this one class.

Changed to a subclass. I added a custom deserializer to handle the different classes.

Okay, so I said this, Adam said this, but after reading the code and our other discussions above timeseries does it make more sense to just make TimeSeries more generic?

e.g.

A builder where you manually add column names, index, and type, and and functions in the row builder to set such?
something like

withColumn(int index, String name, String description, Class<T> type) { "logic" } ... Record: <T> setColumn(int index, T value, Class<T> type) { "logic" }

Or something like that, it would prevent the need in TimeSeriesDaoImpl have two different loops doing almost 90% the same work. just a check for "I have this column requested, let's also add it."

Basically instead of hard coding the columns at all (okay, maybe time... is a time series) , the user of the given TimeSeries object (after set by builder) can define them at run-time.

Sorry, I know you did a lot here, that just came to me now looking through the current PR.

I don't want Zach to do it in this PR, already been too long, but does anyone think the above idea is crazy?

MikeNeilson · 2024-11-04T19:51:04Z

@jbkolze what are your thoughts on this? At least how it's done. Some appears it may be a breaking change which we want to avoid.

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

jbkolze · 2024-11-04T20:28:47Z

@jbkolze what are your thoughts on this? At least how it's done. Some appears it may be a breaking change which we want to avoid.

Conceptually, I don't personally have any qualms with it. You had mentioned in the typing discussion that you all were trying to leave flexibility for dynamically adjusting the time series values array, and this seems like a prime use case for that.

That being said, I am a little concerned about the part you marked as a breaking change. I don't know the CDA source that well, but is this indicating that the response would include a fourth null value even if the include_entry_date is false? Because that definitely would not be ideal -- we've written a lot of CDA code already (and I think other districts have as well) that would have to be updated. Not as big of a deal if an API version were included in the path (.../v3/...), but somewhat cumbersome in the current setup.

My understanding from previous conversations is that you'd get the normal 3-value array if this were set to false, but receive a 4-value array if include_entry_date is true. And the "value-columns" object would be updated to match. That'd be my "ideal" implementation.

zack-rma · 2024-11-04T21:47:19Z

You are correct that setting the include_entry_date parameter to true would result in a four-value array, whereas setting it to false would return a three-value array. Your "ideal" implementation is what I was aiming for to retain backwards compatibility and avoid breaking any of the other endpoints that rely on the TimeSeries implementation.

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

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

cwms-data-api/src/main/java/cwms/cda/formatters/json/adapters/TimeSeriesRecordDeserializer.java

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

MikeNeilson · 2024-11-05T14:43:44Z

My understanding from previous conversations is that you'd get the normal 3-value array if this were set to false, but receive a 4-value array if include_entry_date is true. And the "value-columns" object would be updated to match. That'd be my "ideal" implementation.

That is correct for the location indicated, which is location levels backed by a time series. I don't know how much it would affect what you're currently using, but it's definitely not ideal. At the least it definitely shouldn't be null, may as well provide the data, but seems like a parameter should be added to match on the location level end point.

But it sounds like your on the same page with Adam about the shape being already explicitly flexible so I'm okay with that section now; not "ideal", but what is, definitely something to better document though.

MikeNeilson · 2024-11-05T23:32:48Z

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

-                });
+                logger.fine(() -> query2.getSQL(ParamType.INLINED));
+                final TimeSeriesWithDate timeSeries = new TimeSeriesWithDate(timeseries);
+                query2.forEach(tsRecord -> timeSeries.addValue(


we don't need to solve this now, but definitely before we add requesting any text notes as well. There has to be a better way to handle this with the builders.

Given the previous logic above, does this need a new or just a cast so you can get the desired methods?

MikeNeilson · 2024-11-05T23:34:10Z

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

        if (pageSize != 0) {
+            if (versionDate != null) {
+                whereCond = whereCond.and(AV_TSV_DQU.AV_TSV_DQU.VERSION_DATE.eq(versionDate == null ? null :


Does this logic handle max version? or will AV_TSV_DQU always return every version? or only the specifically requested version?

Updated logic to handle max version in line with previous implementation. If the version date is not specified, the max version will be set to true and the most recent version date will be used for retrieval. This does prevent retrieval of multiple versions in one call, so that could be a problem if that is a desired feature.

MikeNeilson · 2024-11-05T23:43:01Z

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

@@ -248,6 +256,11 @@ public static class Record {
        @JsonProperty(value = "quality-code", index = 2)
        int qualityCode;



Okay, so I said this, Adam said this, but after reading the code and our other discussions above timeseries does it make more sense to just make TimeSeries more generic?

e.g.

A builder where you manually add column names, index, and type, and and functions in the row builder to set such?
something like

withColumn(int index, String name, String description, Class<T> type) { "logic" } ... Record: <T> setColumn(int index, T value, Class<T> type) { "logic" }

Or something like that, it would prevent the need in TimeSeriesDaoImpl have two different loops doing almost 90% the same work. just a check for "I have this column requested, let's also add it."

Basically instead of hard coding the columns at all (okay, maybe time... is a time series) , the user of the given TimeSeries object (after set by builder) can define them at run-time.

Sorry, I know you did a lot here, that just came to me now looking through the current PR.

zack-rma · 2024-12-07T00:08:54Z

Build errors appear to be unrelated to these changes

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

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

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

adamkorynta · 2024-12-18T20:12:48Z

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

+@FormattableWith(contentType = Formats.XMLV2, formatter = XMLv2.class, aliases = {Formats.XML})
+public final class TimeSeriesWithDate extends TimeSeries {
+
+    private List<TimeSeriesWithDate.Record> values;


should be able to reuse the parent's values since there is inheritance

Needed to add the local variables to get the typing correct. Removing it causes loss of type within the List, which drops the data entry date value.

then you shouldn't inherit from TimeSeries as the class structure then has two sets of data

This lends itself to the other suggestion I made with the more generic structure. Though I think that would be too much for this PR.

The Time Series class Daniel created is already generic enough (in concept at least) to handle adding additional columns.

Cleaned up the duplicate value list data from the subclass. Required adding custom deserializer back to properly handle the subtypes of TimeSeries.Record.

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

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

cwms-data-api/src/test/resources/cwms/cda/api/lrl/1day_offset_version_date.json

cwms-data-api/src/test/java/cwms/cda/data/dao/TimeSeriesDaoTest.java

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

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

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

cwms-data-api/src/test/resources/cwms/cda/api/lrl/1day_offset_version_date_roundtrip.json

zack-rma · 2025-02-12T21:50:12Z

Build will fail until database schema changes are merged and a new release is available. Changes can be found here: https://bitbucket.hecdev.net/projects/CWMS/repos/cwms_database/pull-requests/412/overview

MikeNeilson · 2025-03-11T23:49:43Z

Tests are still failing, actions output is still showing schema of "latest-dev"

inguyen314 · 2025-05-19T20:31:01Z

what is status?

zack-rma · 2025-05-19T20:37:43Z

what is status?

The necessary changes to the database were just merged in, we'll need an updated schema to be available before this feature can be finalized.

…on bug in progress

…dle custom output, adds data entry date support

…ned up DTO

…e values with data entry date

Added integration tests with data. Fixed unit parameter usage in integration tests

MikeNeilson

I have some minor nitpicks on formatting.

MikeNeilson · 2025-05-20T14:22:30Z

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

        });

+        if (includeEntryDate) {


why are we doing this check if the above would already have returned a TimeSeriesWithDataEntryDate?

Alternatively why have the login within the lambda for data_entry_date if we're goign to change it here?

(I'm not seeing a difference other than the type after new.)

Remnant from before the above line, perhaps. I've removed it and tested that all works as intended without it.

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

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

-                });
+                logger.fine(() -> query2.getSQL(ParamType.INLINED));
+                final TimeSeriesWithDate timeSeries = new TimeSeriesWithDate(timeseries);
+                query2.forEach(tsRecord -> timeSeries.addValue(


Given the previous logic above, does this need a new or just a cast so you can get the desired methods?

MikeNeilson · 2025-05-20T14:25:45Z

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

-                }
-                tsvArray.add(new ZTSV_TYPE(value.getDateTime(), dataValue, BigDecimal.valueOf(value.getQualityCode())));
-            }
+			for(TimeSeries.Record value : values)


incorrect curlies, but otherwise thank you for cleaning up the loop structures and formatting.

Fixed brackets

MikeNeilson · 2025-05-20T14:27:58Z

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

@@ -248,6 +256,11 @@ public static class Record {
        @JsonProperty(value = "quality-code", index = 2)
        int qualityCode;



I don't want Zach to do it in this PR, already been too long, but does anyone think the above idea is crazy?

MikeNeilson · 2025-05-20T14:28:28Z

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


        public Record(Timestamp dateTime, Double value, int qualityCode) {
+            super();


... what super?

Leftover from when I had created a shared parent class to resolve some serialization issues. Removed

MikeNeilson · 2025-05-20T14:29:38Z

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

+ * The data entry date is the date that the data was entered into the database.
+ */
+@JsonDeserialize(using = JsonDeserializer.None.class)
+public final class TimeSeriesRecordWithDate extends TimeSeries.Record {


This confused me. Should be TimeSeriesRecordWithEntryDate

Renamed as suggested

MikeNeilson · 2025-05-20T14:32:31Z

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

                .header("Authorization",user.toHeaderValue())
                .queryParam("office",officeId)
-                .queryParam("units","cfs")
+                .queryParam("unit","cfs")


It would seem we should really start using the constants we've defined. (Not a ding, pretty sure we're all guilty of that.)

Replaced with Controllers constant where applicable

MikeNeilson · 2025-05-20T14:33:56Z

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

+            .accept(Formats.JSONV2)
+            .header("Authorization", user.toHeaderValue())
+            .queryParam(Controllers.OFFICE, officeId)
+            .queryParam(Controllers.UNIT, "CFS")


Yes, this is definitely the superior way to set those parameters.

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

MikeNeilson · 2025-05-20T14:36:23Z

cwms-data-api/src/test/java/cwms/cda/data/dao/TimeSeriesDaoTest.java

thank you for updating the formatting within this file while you were at it.

…types of Record class

MikeNeilson reviewed Oct 22, 2024

View reviewed changes

zack-rma marked this pull request as ready for review October 25, 2024 18:53

zack-rma requested a review from adamkorynta October 25, 2024 21:04

zack-rma changed the title ~~Fixes #634 - Implemented data entry data option for TS data retrieval~~ Fixes #634 - Implemented data entry date option for TS data retrieval Oct 31, 2024

MikeNeilson reviewed Nov 4, 2024

View reviewed changes

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

adamkorynta requested changes Nov 4, 2024

View reviewed changes

MikeNeilson reviewed Nov 5, 2024

View reviewed changes

zack-rma requested a review from adamkorynta December 2, 2024 22:12

adamkorynta requested changes Dec 18, 2024

View reviewed changes

adamkorynta reviewed Dec 19, 2024

View reviewed changes

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

adamkorynta reviewed Dec 19, 2024

View reviewed changes

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

adamkorynta reviewed Dec 19, 2024

View reviewed changes

cwms-data-api/src/test/resources/cwms/cda/api/lrl/1day_offset_version_date_roundtrip.json Outdated Show resolved Hide resolved

zack-rma requested a review from adamkorynta January 21, 2025 21:51

zack-rma force-pushed the bugfix/ts_data_entry_issue_634 branch from 7ff531a to 67bcd17 Compare April 23, 2025 21:20

zack-rma added 7 commits May 19, 2025 14:49

Implemented data entry data option for TS data retrieval, serializati…

acc4f37

…on bug in progress

Created subclass of TimeSeries Record with custom deserializer to han…

23e38f6

…dle custom output, adds data entry date support

Fixes build error

6186362

Fixes test case failure

b19d972

634 TimeSeries Subclass update

e460d5f

634 TimeSeries Subclass update - added Mixin test

e7f2d9e

Updated max version handling, added test case

3844e60

zack-rma added 8 commits May 19, 2025 14:49

Updated based on Adam's feedback. Added serializer test on file, clea…

c6a7d61

…ned up DTO

Updated based on Adam's feedback. Added error when attempting to stor…

24b1849

…e values with data entry date

Renamed json file, fixed documentation

49f4e0b

Fixed data entry date request body checking

5b6f145

Cleaned up TS retrieval with data entry dates.

eabc472

Added integration tests with data. Fixed unit parameter usage in integration tests

Updated comment

910b6af

Added null check

8b069f4

Added database compatibility check

c0b2a47

zack-rma force-pushed the bugfix/ts_data_entry_issue_634 branch from 0ac074b to c0b2a47 Compare May 19, 2025 22:18

zack-rma requested a review from MikeNeilson May 19, 2025 23:34

MikeNeilson requested changes May 20, 2025

View reviewed changes

zack-rma added 6 commits May 21, 2025 13:08

Replaced strings with constants, cleaned up DAO

49df2d4

Merge branch 'develop' into bugfix/ts_data_entry_issue_634

e8d8c0c

Fixed scoping issue

ce9a30a

Removed duplicate value list, added custom deserializer to handle sub…

b4195a1

…types of Record class

Renamed class

d148933

Fixed broken ratings test

efffc13

		@@ -248,6 +256,11 @@ public static class Record {
		@JsonProperty(value = "quality-code", index = 2)
		int qualityCode;


		public Record(Timestamp dateTime, Double value, int qualityCode) {
		super();

Fixes #634 - Implemented data entry date option for TS data retrieval #927

Are you sure you want to change the base?

Fixes #634 - Implemented data entry date option for TS data retrieval #927

Uh oh!

Conversation

zack-rma commented Oct 22, 2024

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!

MikeNeilson commented Nov 4, 2024

Uh oh!

Uh oh!

jbkolze commented Nov 4, 2024

Uh oh!

zack-rma commented Nov 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MikeNeilson commented Nov 5, 2024

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 Dec 7, 2024

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!

MikeNeilson May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zack-rma commented Feb 12, 2025

Uh oh!

MikeNeilson commented Mar 11, 2025

Uh oh!

inguyen314 commented May 19, 2025

Uh oh!

zack-rma commented May 19, 2025

Uh oh!

MikeNeilson left a comment

Choose a reason for hiding this comment

MikeNeilson May 20, 2025 •

edited

Loading