Time Series Profile Controllers #856

zack-rma · 2024-08-21T23:30:47Z

Implementation of Time Series Profile Definition, Parser, and Instance controllers

zack-rma · 2024-08-23T17:32:03Z

Current TimeSeriesProfileInstanceDao Catalog implementation does not return parameter list for the TimeSeriesProfiles

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

zack-rma · 2024-09-12T18:09:23Z

After discussion with the team, a new data structure for the TSP Instances has been developed that should allow for paging implementation and improved consistency across endpoints that deal with time series data (gate changes, turbines, etc). Here's JSON of that new structure. @MikeNeilson Please take a look and let me know your thoughts.

{
  "time-series-profile": {
    "location-id": {
      "office-id": "SWT",
      "name": "SWAN"
    },
    "description": "Description",
    "parameter-list": [
      "Temp-Water",
      "Depth"
    ],
    "key-parameter": "Depth",
    "reference-ts-id": {
      "office-id": "SWT",
      "name": "SWAN.Depth.Inst.0.0.DSS-Obs"
    }
  },
  "parameter-columns": [
    {
      "parameter": "Temp",
      "ordinal": 1,
      "units": "F"
    },
    {
      "parameter": "Depth",
      "ordinal": 2,
      "units": "m"
    }
  ],
  "data-columns": [
    {
      "name": "value",
      "ordinal": 1,
      "datatype": "java.lang.Double"
    },
    {
      "name": "quality",
      "ordinal": 2,
      "datatype": "java.lang.Integer"
    }
  ],
  "time-series": [
    {
      "1612869582120": [
        [86.5, 0],
        [98.6, 0]
      ],
      "1612869682120": [
        [86.999, 0],
        [98.6, 0]
      ],
      "1612870682120": [
        [],
        [98.6, 0]
      ]
    }
  ],
  "location-time-zone": "PST",
  "version": "DSS-Obs",
  "version-date": 1594786000000,
  "first-date": 1594296000000,
  "last-date": 1752062400000,
  "page-first-date": 1594296000000,
  "page-last-date": 1752062400000,
  "page": 1,
  "page-size": 50,
  "cursor": "cursorData"
}

MikeNeilson · 2024-09-12T20:20:07Z

Looks reasonable to me. Might be good to provide a total field in addition to the page and page-size, would let applications be able to properly render a progress bar.

rma-bryson · 2024-09-12T21:29:39Z

"parameter-list": [ "Temp-Water", "Depth" ], "key-parameter": "Depth",

Thoughts on changing this to something like:

"dependent-parameters": [ "Temp-Water", "Dissolved Oxygen" ], "independent-parameter": "Depth",

Unless key-parameter means something other than specifying the independent variable

MikeNeilson · 2024-09-12T21:31:38Z

I believe it does. One of the parameters is used as basically a reference point for other operations.

…rofile parser columnar data

… errors

…fileController

MikeNeilson

A few comments. The test imply the areas I'm asking already do what's asked for VERSION_DATE, but it wasn't super obvious when looking through the controllers themselves.

MikeNeilson · 2024-11-01T16:39:33Z

...ta-api/src/main/java/cwms/cda/api/timeseriesprofile/TimeSeriesProfileInstanceController.java

                    .getOrDefault(true);
            boolean previous = ctx.queryParamAsClass(PREVIOUS, boolean.class).getOrDefault(false);
            boolean next = ctx.queryParamAsClass(NEXT, boolean.class).getOrDefault(false);
            Instant versionDate = ctx.queryParamAsClass(VERSION_DATE, String.class).getOrDefault(null) == null
-                    ? null : Instant.ofEpochMilli(Long.parseLong(ctx.queryParamAsClass(VERSION_DATE, String.class)
-                    .getOrDefault(null)));
+                    ? null : Instant.parse(ctx.queryParamAsClass(VERSION_DATE, String.class)


Will this take the ISO8601 formats?

Updated to take in ISO 8601 format

MikeNeilson · 2024-11-01T16:41:15Z

.../src/main/java/cwms/cda/api/timeseriesprofile/TimeSeriesProfileInstanceCreateController.java

@@ -62,14 +68,14 @@ public TimeSeriesProfileInstanceCreateController(MetricRegistry metrics) {
            @OpenApiParam(name = OVERRIDE_PROTECTION, type = Boolean.class, description = "Override protection"
                + " for the time series profile instance. Default is false"),
            @OpenApiParam(name = VERSION_DATE, type = Long.class, description = "The version date of the"


The Time Series controller allows the ISO8601 formats, shouldn't we do that here as well?

Updated documentation and added tests, now supports ISO 8601 entry

MikeNeilson · 2024-11-01T16:41:46Z

.../src/main/java/cwms/cda/api/timeseriesprofile/TimeSeriesProfileInstanceDeleteController.java

-                + " time series profile instance. Default is UTC"),
+            @OpenApiParam(name = TIMEZONE, description = "Specifies "
+                    + "the time zone of the values of the begin and end fields. If this field is not specified, "
+                    + "the default time zone of UTC shall be used."),
            @OpenApiParam(name = VERSION_DATE, type = Instant.class, description = "The version date of the"


Same comment as others. regarding version date.

Updated to support ISO8601

MikeNeilson · 2024-11-01T16:43:31Z

.../src/main/java/cwms/cda/api/timeseriesprofile/TimeSeriesProfileInstanceDeleteController.java

-                    .getOrDefault(Instant.now().toEpochMilli()));
-            Instant firstDate = Instant.ofEpochMilli(ctx.queryParamAsClass(DATE, Long.class)
-                    .getOrDefault(Instant.now().toEpochMilli()));
+            Instant versionDate = requiredInstant(ctx, VERSION_DATE);


VersionDate is Long in the OpenAPI annotation, but can be string here.

NOTE: I'm pretty sure this line is correct and this code should be used in the other controllers that process a version date as it centralizes the flexibility.

Changed annotation to Instant. Allows for string entry as a long or ISO 8601 format.

MikeNeilson · 2024-11-01T16:51:06Z

...data-api/src/main/java/cwms/cda/data/dao/timeseriesprofile/TimeSeriesProfileInstanceDao.java

+            SelectHavingStep<Record1<Integer>> count = dsl.select(countDistinct(VIEW_TSV2.DATE_TIME))
+                    .from(VIEW_TSV2)
+                    .where(finalWhereCondition);
+            total = Objects.requireNonNull(count.fetchOne()).value1();


This is an odd use of requireNonNull, why aren't we throwing a more specific exception, or none at all if this condition is okay.

Removed usage, replaced with NullPointerException error handling

MikeNeilson · 2024-11-01T16:58:53Z

...data-api/src/main/java/cwms/cda/data/dao/timeseriesprofile/TimeSeriesProfileInstanceDao.java

+        builder.withPageLastDate(timeList.stream().max(Instant::compareTo).orElse(null));
+        builder.withVersionDate(versionDate);
+        return builder
+                .build();


No a huge deal but this doesn't need to be on a new line.

Moved to same line

…torage

…oller_adam update to latest schema version

MikeNeilson

This seems reasonable to me. That said I've been away not reading code for a week so I'm gonna let someone else do the final approval.

Nothing major jumped out. The parameter look reasonable and the tests seemed only as difficult to follow as the data set requires.

...data-api/src/main/java/cwms/cda/data/dao/timeseriesprofile/TimeSeriesProfileInstanceDao.java

…r' into feature/TimeSeriesProfileController

zack-rma marked this pull request as ready for review September 4, 2024 15:43

zack-rma requested review from rma-psmorris and adamkorynta September 4, 2024 16:30

adamkorynta reviewed Sep 5, 2024

View reviewed changes

cwms-data-api/src/main/java/cwms/cda/formatters/json/JsonV1.java Outdated Show resolved Hide resolved

zack-rma marked this pull request as draft September 18, 2024 15:38

zack-rma marked this pull request as ready for review September 18, 2024 22:33

AndreasChristmann added 19 commits September 19, 2024 10:13

ts profile json

90665dd

ts profile json

9d137d8

ts profile json

b9131e7

ts profile json

6458099

sample time series profile data

7141f7e

time series profile dto's

5aeaa72

time series profile dao's

085e855

time series profile dto's unit test

21b9c8b

time series profile dao integration test

a15f561

time series profile unit test

d0dec1f

added db cleanup

825c55e

moved equals to test classes and removed hashcode

20095a3

reformatted code

5b7fd48

use CWMS-Id for location id

1922694

use CWMSId, TimeSeriesProfile Instance DTO, json and unittest

180bef7

unit test for timeseries profile instance, cleanup code

c371fd7

Timeseries profile instance dao and test, columnar data, timeseries p…

116ff55

…rofile parser columnar data

use Temp-Water instead of Temperature

9536980

change profile time series json to match time series

f3bea82

Fixed assertions and exception handling for bad parser type

1510ba2

zack-rma requested a review from adamkorynta October 24, 2024 17:57

adamkorynta previously approved these changes Oct 24, 2024

View reviewed changes

zack-rma requested a review from MikeNeilson October 24, 2024 20:18

Updated Swagger documentation for TimeSeries profile endpoints

284a6b2

zack-rma dismissed adamkorynta’s stale review via 284a6b2 October 25, 2024 21:12

adamkorynta previously approved these changes Oct 25, 2024

View reviewed changes

Updated tests with parser existence verification, diagnosing TC build…

47807ec

… errors

zack-rma dismissed adamkorynta’s stale review via 47807ec October 28, 2024 23:27

zack-rma added 2 commits October 30, 2024 08:47

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

51eb45a

…fileController

Updated test to fix build error

d494dab

MikeNeilson reviewed Nov 1, 2024

View reviewed changes

zack-rma and others added 7 commits November 4, 2024 10:24

Updated version date input handling

43ad339

Added version date input handling tests

4aab9b4

Updated instance integration test to resolve TC build error

9e71d6c

Updated test case to verify instance is in DB before deletion

0b05e7a

TimeSeries test case fix attempt - fixing build error with instance s…

770d0ef

…torage

update to latest schema version

05a5cce

Merge pull request #2 from adamkorynta/feature/TimeSeriesProfileContr…

e37aaff

…oller_adam update to latest schema version

zack-rma requested a review from MikeNeilson November 13, 2024 17:07

Merge branch 'develop' into feature/TimeSeriesProfileController

d82b008

MikeNeilson reviewed Nov 18, 2024

View reviewed changes

adamkorynta reviewed Nov 18, 2024

View reviewed changes

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

zack-rma added 3 commits November 19, 2024 08:51

Replaced NPE catches with null checks

e61214e

Merge remote-tracking branch 'zack/feature/TimeSeriesProfileControlle…

11525fb

…r' into feature/TimeSeriesProfileController

Merge branch 'develop' into feature/TimeSeriesProfileController

0bed160

adamkorynta approved these changes Nov 19, 2024

View reviewed changes

MikeNeilson approved these changes Nov 21, 2024

View reviewed changes

adamkorynta merged commit 5d10b94 into USACE:develop Nov 21, 2024
4 of 5 checks passed

zack-rma deleted the feature/TimeSeriesProfileController branch February 21, 2025 17:33

Time Series Profile Controllers #856

Time Series Profile Controllers #856

Uh oh!

Conversation

zack-rma commented Aug 21, 2024

Uh oh!

zack-rma commented Aug 23, 2024

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

MikeNeilson commented Sep 12, 2024

Uh oh!

rma-bryson commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeNeilson commented Sep 12, 2024

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!

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!

MikeNeilson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zack-rma commented Sep 12, 2024 •

edited

Loading

rma-bryson commented Sep 12, 2024 •

edited

Loading