Issue 693 - Rating curve retrieval #909

zack-rma · 2024-10-10T23:32:11Z

Fixes #693 - Adds support for retrieving a rating closest to a specified date.

MikeNeilson

Looks good. nitpick about variable/parameter names.

MikeNeilson · 2024-10-11T20:23:18Z

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

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


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

Updated to use effectiveDate and effectiveDateParam for consistency

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.

rma-psmorris · 2024-10-22T15:51:24Z

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

+                Instant startInstant = null;
+                Instant endInstant = null;
+                Result<?> results = connectionResult(dsl, c ->
+                    dsl.select(view.RATING_ID, view.EFFECTIVE_DATE, view.RATING_CODE).from(view)


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

Rewrote the query and removed the difference logic from the DAO

adamkorynta · 2024-11-05T00:28:39Z

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

@@ -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 "


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.

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.

adamkorynta · 2024-11-27T00:13:23Z

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

@@ -437,13 +436,18 @@ 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 {
-        RatingSet ratingSet;
+                                   Instant end, boolean latest) throws IOException, RatingException {


Don't add a boolean to a method to change the implementation, just create a new method

Created a new method for latest rating retrieval

zack-rma · 2024-12-07T00:00:00Z

Build failures appear to be unrelated to this PR

adamkorynta · 2024-12-07T01:17:32Z

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

            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, false);


The changes to this controller should be gone right? the latest option is moved to RatingLatestController

Correct, I moved any remnants to the RatingLatestController and cleaned up the original controller

adamkorynta · 2024-12-07T01:18:22Z

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

+import org.jooq.DSLContext;
+
+
+public class RatingLatestController extends RatingController implements Handler {


The inheritance doesn't make sense here, this controller is calling a different DAO method

Removed unnecessary inheritance

adamkorynta · 2024-12-07T01:24:39Z

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

@@ -83,11 +101,96 @@ private static String extractOfficeId(String ratingSet) throws JsonProcessingExc
        return office;
    }

+    public RatingSet retrieveLatest(RatingSet.DatabaseLoadMethod method, String officeId,


Isn't this what cwms_rating.retrieve_eff_ratings_xml does? Can you verify with Mike Perryman?

I'll double check with Perryman

DISREGARD THE PREVIOUS COMMENT

CORRECT COMMENT

Yes you can use cwms_rating.retrieve_eff_ratings_xml_f() to get the most recent rating by setting both p_start_date and p_end_date to sysdate (or whatever time you want the most recent rating for).

MikeNeilson · 2024-12-09T17:14:48Z

Build failures appear to be unrelated to this PR

Yep, it's the usual failures from other end points.

…tController

zack-rma added 3 commits September 30, 2024 16:18

Issue 693 - Update to allow finding closest effective date in progress

142fc4f

Issue 693 - Retrieve most recent rating

1f3a956

Issue 693 - Updated recent rating retrieval, improved test cases

6d92679

zack-rma marked this pull request as ready for review October 11, 2024 17:14

zack-rma requested review from adamkorynta and RyanM-RMA October 11, 2024 17:14

MikeNeilson requested changes Oct 11, 2024

View reviewed changes

Issue 693 - Updated effectiveDate variable naming

e300f64

rma-psmorris reviewed Oct 22, 2024

View reviewed changes

Rewrote closest date finder query, removed logic from CDA

412716e

zack-rma requested review from MikeNeilson and rma-psmorris October 25, 2024 21:01

adamkorynta removed their request for review October 31, 2024 22:50

adamkorynta requested changes Nov 5, 2024

View reviewed changes

Added new latest rating endpoint with tests

7b0124e

zack-rma requested a review from adamkorynta November 26, 2024 23:29

adamkorynta reviewed Nov 27, 2024

View reviewed changes

Turned combined rating retrieval with boolean arg into two methods

b64a83a

zack-rma requested a review from adamkorynta December 2, 2024 18:01

adamkorynta requested changes Dec 7, 2024

View reviewed changes

Removed latest retrieval from RatingsController, moved to RatingLates…

edc5efb

…tController

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 693 - Rating curve retrieval #909

Issue 693 - Rating curve retrieval #909

zack-rma commented Oct 10, 2024 •

edited

Loading

MikeNeilson left a comment

MikeNeilson Oct 11, 2024

zack-rma Oct 11, 2024

adamkorynta Nov 5, 2024

rma-psmorris Oct 22, 2024

zack-rma Oct 23, 2024

adamkorynta Nov 5, 2024

zack-rma Nov 25, 2024

adamkorynta Nov 27, 2024

zack-rma Dec 2, 2024

zack-rma commented Dec 7, 2024

adamkorynta Dec 7, 2024

zack-rma Dec 9, 2024

adamkorynta Dec 7, 2024

zack-rma Dec 9, 2024

adamkorynta Dec 7, 2024

zack-rma Dec 9, 2024

perrymanmd Dec 11, 2024 •

edited

Loading

MikeNeilson commented Dec 9, 2024

		import org.jooq.DSLContext;


		public class RatingLatestController extends RatingController implements Handler {

Issue 693 - Rating curve retrieval #909

Are you sure you want to change the base?

Issue 693 - Rating curve retrieval #909

Conversation

zack-rma commented Oct 10, 2024 • edited Loading

MikeNeilson left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrymanmd Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

DISREGARD THE PREVIOUS COMMENT

CORRECT COMMENT

MikeNeilson commented Dec 9, 2024

zack-rma commented Oct 10, 2024 •

edited

Loading

perrymanmd Dec 11, 2024 •

edited

Loading