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

…on 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.

…dle custom output, adds data entry date support

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.

adamkorynta · 2024-11-04T23:44:16Z

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

-                            tsRecord.getValue(qualityNormCol).intValue()
-                    )
-            );
+            if (includeEntryDate) {


This query is doubling the time it takes to retrieve time series. Can this replace the retrieve_ts_out_tab calls?

While it could replace the retrieve_ts_out_tab call above, doing so would require implementing trim support into the query, as that is currently handled by the retrieve_ts_out_tab call. I haven't quite figured out the best way to do so, so maybe we can discuss this in more detail

if the schema needs to provide more, we can update the schema call

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.

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.

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

adamkorynta · 2024-12-18T20:15:44Z

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

Post/Patch should throw an error if the client is sending a time series with a data-entry-date as that field isn't editable

Added a check for data entry date values

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

…ned up DTO

…e values with data entry date

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

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

95361d7

…on bug in progress

MikeNeilson reviewed Oct 22, 2024

View reviewed changes

Created subclass of TimeSeries Record with custom deserializer to han…

e04b7e4

…dle custom output, adds data entry date support

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

zack-rma added 2 commits November 4, 2024 11:33

Merge branch 'develop' into bugfix/ts_data_entry_issue_634

6b9a89d

Fixes build error

b91145c

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

Fixes test case failure

20615da

adamkorynta requested changes Nov 4, 2024

View reviewed changes

zack-rma added 2 commits November 5, 2024 14:52

634 TimeSeries Subclass update

1d0d389

634 TimeSeries Subclass update - added Mixin test

667248d

MikeNeilson reviewed Nov 5, 2024

View reviewed changes

zack-rma added 3 commits November 25, 2024 15:01

Merge branch 'develop' into bugfix/ts_data_entry_issue_634

a4d4394

Updated max version handling, added test case

d6bcf0a

Merge branch 'develop' into bugfix/ts_data_entry_issue_634

3358bef

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

Merge branch 'develop' into bugfix/ts_data_entry_issue_634

8a7c600

adamkorynta requested changes Dec 18, 2024

View reviewed changes

zack-rma added 2 commits December 18, 2024 16:48

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

9ce5604

…ned up DTO

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

b7bfbf3

…e values with data entry date

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 added 2 commits December 19, 2024 16:09

Renamed json file, fixed documentation

f73866a

Fixed data entry date request body checking

ff688e5

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zack-rma commented Oct 22, 2024

MikeNeilson Oct 22, 2024

zack-rma Oct 25, 2024

MikeNeilson Nov 5, 2024

MikeNeilson commented Nov 4, 2024

jbkolze commented Nov 4, 2024

zack-rma commented Nov 4, 2024

adamkorynta Nov 4, 2024

zack-rma Nov 5, 2024

adamkorynta Dec 19, 2024

MikeNeilson commented Nov 5, 2024

MikeNeilson Nov 5, 2024

MikeNeilson Nov 5, 2024

zack-rma Dec 2, 2024

MikeNeilson Nov 5, 2024

zack-rma commented Dec 7, 2024

adamkorynta Dec 18, 2024

zack-rma Dec 19, 2024

adamkorynta Dec 18, 2024

zack-rma Dec 19, 2024

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

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

Conversation

zack-rma commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeNeilson commented Nov 4, 2024

jbkolze commented Nov 4, 2024

zack-rma commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeNeilson commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zack-rma commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment