Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
95361d7
Implemented data entry data option for TS data retrieval, serializati…
zack-rma Oct 22, 2024
e04b7e4
Created subclass of TimeSeries Record with custom deserializer to han…
zack-rma Oct 25, 2024
6b9a89d
Merge branch 'develop' into bugfix/ts_data_entry_issue_634
zack-rma Nov 4, 2024
b91145c
Fixes build error
zack-rma Nov 4, 2024
20615da
Fixes test case failure
zack-rma Nov 4, 2024
1d0d389
634 TimeSeries Subclass update
zack-rma Nov 5, 2024
667248d
634 TimeSeries Subclass update - added Mixin test
zack-rma Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions cwms-data-api/src/main/java/cwms/cda/api/TimeSeriesController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@
import io.javalin.plugin.openapi.annotations.OpenApiParam;
import io.javalin.plugin.openapi.annotations.OpenApiRequestBody;
import io.javalin.plugin.openapi.annotations.OpenApiResponse;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
Expand All @@ -84,7 +83,7 @@

public class TimeSeriesController implements CrudHandler {
private static final Logger logger = Logger.getLogger(TimeSeriesController.class.getName());

private static final String INCLUDE_ENTRY_DATE = "include-entry-date";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a purely pedantic standpoint this should really be a parameter of the content-type, but I may have to accept the reality of this being easier for everyone.

@krowvin, we were just discussing this conceptually.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path parameters are the "what" I am retrieving, query parameters are the "how" am I adding to or filtering that data, and content-type is the "shape" that is returned. This change effects the how and given the way the column names and data array are paired together to already give this flexibility, I don't see a change in shape.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a column is definitely a change in shape. The what is a time series, the query parameters specify exactly which time series, or at least which portion of a given time series (I guess if we're being really pedantic begin and end should be in the fragment... but I digress).

While the flexibility is there, it's flexibility to change the shape. I don't totally disagree with you but given we haven't communicated that portion of the contract very well we are introducing a breaking change. We already have more than one downstream library dependent on these types.

I'm going to type up something on the wiki, or maybe discussion, for the philosophy I'm going for with these, hopefully my argument makes more sense in regards to query vs content type. Especially as it relates to some of the challenges we're currently seeing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is not conveyed very well that one should use the columns array to determine which index to grab from the data array. I don't think that adding information to the swagger docs to communicate that should trigger a content-type change though (maybe version=2.5 but that seems like a huge headache) especially since not including the parameter to retrieve entry date keeps the array intact for backwards compatibility.

Also, I've never seen an API where the content-type changes how much extra (or less) data gets returned to the client. I'd like to see some examples. I also don't see the reason (pedantic or not) for adding begin/end as path parameters as those are filters on the time series. Everything for the identifier of the time series encompasses the time series (which is also why the date version shouldn't be in the path and is a query parameter).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: not arguing for it, but explanation of my logic on the fragement.
a time series is:
/timeseries/Alder Springs.Precip-INC.Total.1Hour.1Hour.Calc?version_date=unversioned identifies a specific time series which is a mathematical series of data - and technically all of it. The entire series could be consider a "document", a fragment is a section within a document; traditionally we would think of lines in a file, but it applies to the time series as well. A file, after all, is just a series of lines.

As I said, extremely pedantic.... admittedly almost to the point of being useless because literally no one does it that way, nor would they even if it could be proven objectively correct.

Technically the units are also representation and not identification but given how limited the use of content-type features are used it would be incredibly difficult to get people to use it; I don't even think the Swagger-UI has a mechanism to slightly tweak the content-type.

But back on the topic of what's correct for us, it seems we're all in agreement that @DanielTOsborne 's initial design is already sufficiently flexible in the current scheme and our failure was in how we documented that for the general end user.

So we leave the inclusion as a query parameter unless a better way actually comes along.

public static final String TAG = "TimeSeries";
public static final String STORE_RULE_DESC = "The business rule to use "
+ "when merging the incoming with existing data\n"
Expand Down Expand Up @@ -204,7 +203,7 @@ public void create(@NotNull Context ctx) {
TimeSeries timeSeries = deserializeTimeSeries(ctx);
dao.create(timeSeries, createAsLrts, storeRule, overrideProtection);
ctx.status(HttpServletResponse.SC_OK);
} catch (IOException | DataAccessException ex) {
} catch (DataAccessException ex) {
CdaError re = new CdaError("Internal Error");
logger.log(Level.SEVERE, re.toString(), ex);
ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re);
Expand Down Expand Up @@ -382,6 +381,8 @@ public void delete(@NotNull Context ctx, @NotNull String timeseries) {
+ "\n* `xml`"
+ "\n* `wml2` (only if name field is specified)"
+ "\n* `json` (default)"),
@OpenApiParam(name = INCLUDE_ENTRY_DATE, type = Boolean.class, description = "Specifies "
+ "whether to include the data entry date in the response. Default is false."),
@OpenApiParam(name = PAGE, description = "This end point can return large amounts "
+ "of data as a series of pages. This parameter is used to describes the "
+ "current location in the response stream. This is an opaque "
Expand Down Expand Up @@ -431,6 +432,9 @@ public void getAll(@NotNull Context ctx) {

ZonedDateTime versionDate = queryParamAsZdt(ctx, VERSION_DATE);

boolean includeEntryDate = ctx.queryParamAsClass(INCLUDE_ENTRY_DATE, Boolean.class)
.getOrDefault(false);

// The following parameters are only used for jsonv2 and xmlv2
String cursor = queryParamAsClass(ctx, new String[]{PAGE, CURSOR},
String.class, "", metrics, name(TimeSeriesController.class.getName(),
Expand Down Expand Up @@ -463,7 +467,7 @@ public void getAll(@NotNull Context ctx) {

String office = requiredParam(ctx, OFFICE);
TimeSeries ts = dao.getTimeseries(cursor, pageSize, names, office, unit,
beginZdt, endZdt, versionDate, trim.getOrDefault(true));
beginZdt, endZdt, versionDate, trim.getOrDefault(true), includeEntryDate);

results = Formats.format(contentType, ts);

Expand Down Expand Up @@ -573,14 +577,14 @@ public void update(@NotNull Context ctx, @NotNull String id) {
dao.store(timeSeries, createAsLrts, storeRule, overrideProtection);

ctx.status(HttpServletResponse.SC_OK);
} catch (IOException | DataAccessException ex) {
} catch (DataAccessException ex) {
CdaError re = new CdaError("Internal Error");
logger.log(Level.SEVERE, re.toString(), ex);
ctx.status(HttpServletResponse.SC_INTERNAL_SERVER_ERROR).json(re);
}
}

private TimeSeries deserializeTimeSeries(Context ctx) throws IOException {
private TimeSeries deserializeTimeSeries(Context ctx) {
String contentTypeHeader = ctx.req.getContentType();
ContentType contentType = Formats.parseHeader(contentTypeHeader, TimeSeries.class);
return Formats.parseContent(contentType, ctx.bodyAsInputStream(), TimeSeries.class);
Expand Down
3 changes: 1 addition & 2 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/LocationLevelsDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TimeZone;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -623,7 +622,7 @@ private static TimeSeries buildTimeSeries(ILocationLevelRef levelRef, Interval i
if (qualityCode != null) {
quality = qualityCode.intValue();
}
timeSeries.addValue(dateTime, value, quality);
timeSeries.addValue(dateTime, value, quality, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this isn't a breaking change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check with additional test cases, but this change should not break any existing functionality. A null data entry date parameter is treated as if a standard Time-Value-Quality data entry was provided. The implementation of addValue will use a TimeSeries data record with only three input fields under normal circumstances:

if (dataEntryDate != null) { values.add(new TimeSeriesRecordWithDate(dateTime, value, qualityCode, dataEntryDate)); } else { values.add(new Record(dateTime, value, qualityCode)); }

The existing use cases of TimeSeries should be unaffected, as they will be handled exactly as they were before these changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend subclassing TimeSeries instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would break CWMS.js, The javascript openapi generator already has trouble with our TimeSeries class given some specific assumptions the generator chose to make.

}
return timeSeries;
}
Expand Down
2 changes: 1 addition & 1 deletion cwms-data-api/src/main/java/cwms/cda/data/dao/TimeSeriesDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ void store(TimeSeries timeSeries, boolean createAsLrts,

TimeSeries getTimeseries(String cursor, int pageSize, String names, String office,
String unit, ZonedDateTime begin, ZonedDateTime end,
ZonedDateTime versionDate, boolean trim);
ZonedDateTime versionDate, boolean trim, boolean includeEntryDate);

String getTimeseries(String format, String names, String office, String unit, String datum,
ZonedDateTime begin, ZonedDateTime end, ZoneId timezone);
Expand Down
63 changes: 53 additions & 10 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/TimeSeriesDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import org.jooq.Record;
import org.jooq.Record1;
import org.jooq.Record3;
import org.jooq.Record4;
import org.jooq.Record7;
import org.jooq.Result;
import org.jooq.SQL;
Expand Down Expand Up @@ -165,7 +166,7 @@ public String getTimeseries(String format, String names, String office, String u
public TimeSeries getTimeseries(String page, int pageSize, String names, String office,
String units,
ZonedDateTime beginTime, ZonedDateTime endTime,
ZonedDateTime versionDate, boolean shouldTrim) {
ZonedDateTime versionDate, boolean shouldTrim, boolean includeEntryDate) {
TimeSeries retVal = null;
String cursor = null;
Timestamp tsCursor = null;
Expand Down Expand Up @@ -237,7 +238,7 @@ public TimeSeries getTimeseries(String page, int pageSize, String names, String

// put all those columns together as "valid"
CommonTableExpression<Record7<BigDecimal, String, String, String, String, BigDecimal,
String>> valid =
String>> valid =
name("valid").fields("tscode", "tsid", "office_id", "loc_part", "units",
"interval", "parm_part")
.as(
Expand All @@ -249,7 +250,6 @@ public TimeSeries getTimeseries(String page, int pageSize, String names, String
unit.as("units"),
ival.as("interval"),
param.as("parm_part")

).from(validTs)
);

Expand Down Expand Up @@ -369,6 +369,8 @@ public TimeSeries getTimeseries(String page, int pageSize, String names, String
);
});

Field<Timestamp> dataEntryDate = field("DATA_ENTRY_DATE", Timestamp.class).as("data_entry_date");

if (pageSize != 0) {
SelectConditionStep<Record3<Timestamp, Double, BigDecimal>> query =
dsl.select(
Expand All @@ -391,14 +393,55 @@ public TimeSeries getTimeseries(String page, int pageSize, String names, String
query.limit(DSL.val(pageSize + 1));
}

logger.fine(() -> query.getSQL(ParamType.INLINED));
SelectConditionStep<Record3<Timestamp, Double, BigDecimal>> finalQuery = query;
logger.fine(() -> finalQuery.getSQL(ParamType.INLINED));

query.forEach(tsRecord -> timeseries.addValue(
tsRecord.getValue(dateTimeCol),
tsRecord.getValue(valueCol),
tsRecord.getValue(qualityNormCol).intValue()
)
);
if (includeEntryDate) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SelectConditionStep<Record4<Timestamp, Double, BigDecimal, Timestamp>> query2 = dsl.select(
dateTimeCol,
valueCol,
qualityNormCol,
dataEntryDate
)
.from(AV_TSV_DQU.AV_TSV_DQU)
.where(dateTimeCol
.greaterOrEqual(CWMS_UTIL_PACKAGE.call_TO_TIMESTAMP__2(
DSL.nvl(DSL.val(tsCursor == null ? null :
tsCursor.toInstant().toEpochMilli()),
DSL.val(beginTime.toInstant().toEpochMilli())))))
.and(dateTimeCol
.lessOrEqual(CWMS_UTIL_PACKAGE.call_TO_TIMESTAMP__2(
DSL.val(endTime.toInstant().toEpochMilli())))
.and(AV_TSV_DQU.AV_TSV_DQU.CWMS_TS_ID.equalIgnoreCase(names))
.and(AV_TSV_DQU.AV_TSV_DQU.OFFICE_ID.eq(office))
.and(AV_TSV_DQU.AV_TSV_DQU.UNIT_ID.equalIgnoreCase(unit))
.and(AV_TSV_DQU.AV_TSV_DQU.VERSION_DATE.eq(versionDate == null ? null :
Timestamp.from(versionDate.toInstant())))
);

if (pageSize > 0) {
query2.limit(DSL.val(pageSize + 1));
}
query2.forEach(tsRecord -> {
assert timeseries != null;
timeseries.addValue(
tsRecord.getValue(dateTimeCol),
tsRecord.getValue(valueCol),
tsRecord.getValue(qualityNormCol).intValue(),
tsRecord.getValue(dataEntryDate)
);
});
} else {
query.forEach(tsRecord -> {
assert timeseries != null;
timeseries.addValue(
tsRecord.getValue(dateTimeCol),
tsRecord.getValue(valueCol),
tsRecord.getValue(qualityNormCol).intValue(),
null
);
});
}

retVal = timeseries;
}
Expand Down
Loading
Loading