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

Issue 693 - Rating curve retrieval #909

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

Issue 693 - Rating curve retrieval #909

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
142fc4f
Issue 693 - Update to allow finding closest effective date in progress
zack-rma Sep 30, 2024
1f3a956
Issue 693 - Retrieve most recent rating
zack-rma Oct 10, 2024
6d92679
Issue 693 - Updated recent rating retrieval, improved test cases
zack-rma Oct 11, 2024
e300f64
Issue 693 - Updated effectiveDate variable naming
zack-rma Oct 11, 2024
412716e
Rewrote closest date finder query, removed logic from CDA
zack-rma Oct 23, 2024
7b0124e
Added new latest rating endpoint with tests
zack-rma Nov 25, 2024
b64a83a
Turned combined rating retrieval with boolean arg into two methods
zack-rma Dec 2, 2024
edc5efb
Removed latest retrieval from RatingsController, moved to RatingLates…
zack-rma Dec 9, 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
41 changes: 25 additions & 16 deletions cwms-data-api/src/main/java/cwms/cda/api/RatingController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import static cwms.cda.api.Controllers.DATE_FORMAT;
import static cwms.cda.api.Controllers.DATUM;
import static cwms.cda.api.Controllers.DELETE;
import static cwms.cda.api.Controllers.EFFECTIVE_DATE;
import static cwms.cda.api.Controllers.END;
import static cwms.cda.api.Controllers.EXAMPLE_DATE;
import static cwms.cda.api.Controllers.FORMAT;
Expand Down Expand Up @@ -296,19 +297,16 @@ public void getAll(@NotNull Context ctx) {

ContentType contentType = Formats.parseHeaderAndQueryParm(header, format, RatingAliasMarker.class);

if (format.isEmpty())
{
if (format.isEmpty()) {
//Use the full content type here (i.e. application/json;version=2)
ctx.contentType(contentType.toString());
}
else
{
} else {
//Legacy content type only includes the basic type (i.e. application/json)
ctx.contentType(contentType.getType());
}

//At the moment, we still use the legacy formatting here, since we don't have a newer API for serializing/deserializing
//a collection of rating sets - unlike getOne.
//At the moment, we still use the legacy formatting here, since we don't have a newer API for
// serializing/deserializing a collection of rating sets - unlike getOne.
String legacyFormat = Formats.getLegacyTypeFromContentType(contentType);
String results = ratingDao.retrieveRatings(legacyFormat, names, unit, datum, office, start,
end, timezone);
Expand All @@ -322,7 +320,8 @@ public void getAll(@NotNull Context ctx) {

@OpenApi(
pathParams = {
@OpenApiParam(name = RATING_ID, required = true, description = "The rating-id of the effective dates to be retrieve. "),
@OpenApiParam(name = RATING_ID, required = true, description = "The rating-id of the effective "
+ "dates to be retrieve. "),
},
queryParams = {
@OpenApiParam(name = OFFICE, required = true, description =
Expand All @@ -339,6 +338,10 @@ public void getAll(@NotNull Context ctx) {
+ "otherwise specified), as well as the time zone of any times in the"
+ " response. If this field is not specified, the default time zone "
+ "of UTC shall be used."),
@OpenApiParam(name = EFFECTIVE_DATE, description = "Specifies the "
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a very confusing parameter.

  • three optional date parameters are confusing
  • what happens when all three are input?
  • the ask was for the current effective date, that means the client needs to always set this parameter to now. Why not just have another endpoint that does it automatically? /{rating-id}/latest-effective
  • the "closest" effective date shouldn't return a newer effective date than what is supplied as the newer effective date is not yet in effect, though this is made more complicated by temporal interpolations and transition dates.
  • how do active ratings play an effect? I presume they would be skipped, but that's missing from the docs (even from the start/end parameters)

Additionally, you are passing this confusion into the DAO by expanding the existing DAO method instead of creating a new one, so now you have three dates in the DAO function. This makes test cases far more complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved functionality to its own endpoint to reduce confusion. New endpoint returns closest effective date rating where the date is not beyond the current date.

+ "date to find the closest match to for retrieving a specific rating curve. This date is used "
+ "instead of the time window specified by start and end. "
+ "The format for this field is ISO 8601 extended."),
@OpenApiParam(name = METHOD, description = "Specifies "
+ "the retrieval method used. If no method is provided EAGER will be used.",
type = RatingSet.DatabaseLoadMethod.class),
Expand All @@ -354,8 +357,13 @@ public void getAll(@NotNull Context ctx) {
public void getOne(@NotNull Context ctx, @NotNull String rating) {

try (final Timer.Context ignored = markAndTime(GET_ONE)) {
String officeId = ctx.queryParam(OFFICE);

String timezone = ctx.queryParamAsClass(TIMEZONE, String.class).getOrDefault("UTC");
Instant date = null;
String specificDate = ctx.queryParam(EFFECTIVE_DATE);
Copy link
Contributor

Choose a reason for hiding this comment

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

use effectiveDate throughout. it's confusing having this go from EFFECTIVE_DATE, to specificDate, to just "date" in the later code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use effectiveDate and effectiveDateParam for consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

While consistency is good, I think this illuminates that the parameter is confusing on this endpoint. I recommend using a separate endpoint. I might be missing a use case where a the closest effective date to a parameter is needed, but given temporal interpolation and transitional dates, that seems to be too complicated.

if (specificDate != null) {
date = DateUtils.parseUserDate(specificDate, timezone).toInstant();
}

Instant beginInstant = null;
String begin = ctx.queryParam(BEGIN);
Expand All @@ -369,11 +377,13 @@ public void getOne(@NotNull Context ctx, @NotNull String rating) {
endInstant = DateUtils.parseUserDate(end, timezone).toInstant();
}

String officeId = ctx.queryParam(OFFICE);

RatingSet.DatabaseLoadMethod method = ctx.queryParamAsClass(METHOD,
RatingSet.DatabaseLoadMethod.class)
.getOrDefault(RatingSet.DatabaseLoadMethod.EAGER);

String body = getRatingSetString(ctx, method, officeId, rating, beginInstant, endInstant);
String body = getRatingSetString(ctx, method, officeId, rating, beginInstant, endInstant, date);
if (body != null) {
ctx.result(body);
ctx.status(HttpCode.OK);
Expand All @@ -385,7 +395,7 @@ public void getOne(@NotNull Context ctx, @NotNull String rating) {
@Nullable
private String getRatingSetString(Context ctx, RatingSet.DatabaseLoadMethod method,
String officeId, String rating, Instant begin,
Instant end) {
Instant end, Instant date) {
String retval = null;

try (final Timer.Context ignored = markAndTime("getRatingSetString")) {
Expand All @@ -398,7 +408,7 @@ private String getRatingSetString(Context ctx, RatingSet.DatabaseLoadMethod meth
if (isJson || isXml) {
ctx.contentType(contentType.toString());
try {
RatingSet ratingSet = getRatingSet(ctx, method, officeId, rating, begin, end);
RatingSet ratingSet = getRatingSet(ctx, method, officeId, rating, begin, end, date);
if (ratingSet != null) {
if (isJson) {
retval = JsonRatingUtils.toJson(ratingSet);
Expand All @@ -424,8 +434,7 @@ private String getRatingSetString(Context ctx, RatingSet.DatabaseLoadMethod meth
} else {
CdaError re = new CdaError("Currently supporting only: " + Formats.JSONV2
+ " and " + Formats.XMLV2);
logger.log(Level.WARNING, "Provided accept header not recognized:"
+ acceptHeader, re);
logger.log(Level.WARNING, String.format("Provided accept header not recognized: %s", acceptHeader), re);
ctx.status(HttpServletResponse.SC_NOT_IMPLEMENTED);
ctx.json(CdaError.notImplemented());
}
Expand All @@ -437,13 +446,13 @@ private String getRatingSetString(Context ctx, RatingSet.DatabaseLoadMethod meth

private RatingSet getRatingSet(Context ctx, RatingSet.DatabaseLoadMethod method,
String officeId, String rating, Instant begin,
Instant end) throws IOException, RatingException {
Instant end, Instant date) throws IOException, RatingException {
RatingSet ratingSet;
try (final Timer.Context ignored = markAndTime("getRatingSet")) {
DSLContext dsl = getDslContext(ctx);

RatingDao ratingDao = getRatingDao(dsl);
ratingSet = ratingDao.retrieve(method, officeId, rating, begin, end);
ratingSet = ratingDao.retrieve(method, officeId, rating, begin, end, date);
}

return ratingSet;
Expand Down
2 changes: 1 addition & 1 deletion cwms-data-api/src/main/java/cwms/cda/data/dao/RatingDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public interface RatingDao {
void create(String ratingSet, boolean storeTemplate) throws IOException, RatingException;

RatingSet retrieve(RatingSet.DatabaseLoadMethod method, String officeId, String specificationId,
Instant start, Instant end) throws IOException, RatingException;
Instant start, Instant end, Instant date) throws IOException, RatingException;

String retrieveRatings(String format, String names, String unit, String datum, String office,
String start, String end, String timezone);
Expand Down
69 changes: 53 additions & 16 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/RatingSetDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,20 @@
import com.fasterxml.jackson.dataformat.xml.XmlMapper;
import hec.data.RatingException;
import hec.data.cwmsRating.RatingSet;
import java.io.IOException;
import java.sql.Connection;
import java.sql.Timestamp;
import java.time.Instant;
import java.time.ZoneId;
import java.util.List;
import mil.army.usace.hec.cwms.rating.io.jdbc.ConnectionProvider;
import mil.army.usace.hec.cwms.rating.io.jdbc.RatingJdbcFactory;
import org.jooq.DSLContext;
import org.jooq.Result;
import org.jooq.exception.DataAccessException;
import usace.cwms.db.jooq.codegen.packages.CWMS_RATING_PACKAGE;
import usace.cwms.db.jooq.codegen.tables.AV_RATING_LOCAL;

import java.io.IOException;
import java.sql.Connection;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.List;

public class RatingSetDao extends JooqDao<RatingSet> implements RatingDao {

Expand All @@ -57,8 +60,7 @@ public void create(String ratingSetXml, boolean storeTemplate) throws IOExceptio
DSLContext context = getDslContext(c, office);
String errs = CWMS_RATING_PACKAGE.call_STORE_RATINGS_XML__5(context.configuration(),
ratingSetXml, "T", storeTemplate ? "T" : "F");
if (errs != null && !errs.isEmpty())
{
if (errs != null && !errs.isEmpty()) {
throw new DataAccessException(errs);
}
});
Expand All @@ -85,8 +87,9 @@ private static String extractOfficeId(String ratingSet) throws JsonProcessingExc

@Override
public RatingSet retrieve(RatingSet.DatabaseLoadMethod method, String officeId,
String specificationId, Instant startZdt, Instant endZdt
String specificationId, Instant startZdt, Instant endZdt, Instant date
) throws IOException, RatingException {
AV_RATING_LOCAL view = AV_RATING_LOCAL.AV_RATING_LOCAL;

final RatingSet[] retval = new RatingSet[1];
try {
Expand All @@ -110,9 +113,43 @@ public RatingSet retrieve(RatingSet.DatabaseLoadMethod method, String officeId,

RatingSet.DatabaseLoadMethod finalMethod = method;

connection(dsl, c -> retval[0] =
RatingJdbcFactory.ratingSet(finalMethod, new RatingConnectionProvider(c), officeId,
specificationId, start, end, false));
if (date != null) {

String ratingId = null;
Long difference = null;
Instant startInstant = null;
Instant endInstant = null;
Result<?> results = connectionResult(dsl, c ->
dsl.select(view.RATING_ID, view.EFFECTIVE_DATE, view.RATING_CODE).from(view)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be possible to have this query do the limit rather than pulling all records and do tinghe limit in the CDA code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rewrote the query and removed the difference logic from the DAO

.where(view.OFFICE_ID.eq(officeId)
.and(view.RATING_ID.eq(specificationId))
).fetch()
);
for (int i = 0; i < results.size(); i++) {
Timestamp effectiveDate = results.getValue(i, view.EFFECTIVE_DATE);
long effectiveDifference = Math.abs(effectiveDate.toInstant().toEpochMilli() - date.toEpochMilli());
if (difference == null || effectiveDifference < difference) {
difference = effectiveDifference;
ratingId = results.getValue(i, view.RATING_ID);
startInstant = effectiveDate.toInstant().atZone(ZoneId.of("UTC")).toInstant();
endInstant = effectiveDate.toInstant().atZone(ZoneId.of("UTC")).toInstant();
}
}
if (ratingId == null) {
return null;
}
final Instant finalStartInstant = startInstant;
final String ratingIdFinal = ratingId;
final Instant finalEndInstant = endInstant;
connection(dsl, c -> retval[0] =
RatingJdbcFactory.ratingSet(finalMethod, new RatingConnectionProvider(c), officeId,
ratingIdFinal, finalStartInstant.toEpochMilli(),
finalEndInstant.toEpochMilli(), false));
} else {
connection(dsl, c -> retval[0] =
RatingJdbcFactory.ratingSet(finalMethod, new RatingConnectionProvider(c), officeId,
specificationId, start, end, false));
}

} catch (DataAccessException ex) {
Throwable cause = ex.getCause();
Expand Down Expand Up @@ -151,7 +188,7 @@ public void store(String ratingSetXml, boolean includeTemplate) throws IOExcepti
public void delete(String officeId, String specificationId, Instant start, Instant end) {
Timestamp startDate = new Timestamp(start.toEpochMilli());
Timestamp endDate = new Timestamp(end.toEpochMilli());
dsl.connection(c->
dsl.connection(c ->
CWMS_RATING_PACKAGE.call_DELETE_RATINGS(
getDslContext(c,officeId).configuration(), specificationId, startDate,
endDate, "UTC", officeId
Expand All @@ -170,15 +207,15 @@ public String retrieveRatings(String format, String names, String unit, String d
}

private static final class RatingConnectionProvider implements ConnectionProvider {
private final Connection c;
private final Connection conn;

private RatingConnectionProvider(Connection c) {
this.c = c;
private RatingConnectionProvider(Connection conn) {
this.conn = conn;
}

@Override
public Connection getConnection() {
return c;
return conn;
}

@Override
Expand Down
Loading
Loading