Skip to content

CDA-140: Enforce default row limit and validation for AccountingCatalogControl… #1999

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

Open
RyanM-RMA wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

CDA-140: Enforce default row limit and validation for AccountingCatalogControl… #1999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfd94a3
Enforce default row limit and validation for AccountingCatalogControl…
RyanM-RMA Oct 9, 2026
b6c9723
Adding integration test for validating 0 row limit produces a bad req…
RyanM-RMA Oct 16, 2026
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
13 changes: 11 additions & 2 deletions cwms-data-api/src/main/java/cwms/cda/api/watersupply/AccountingCatalogController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public class AccountingCatalogController implements Handler {
private static final String TAG = "Pump Accounting";
private static final String ROW_LIMIT = "row-limit";
private static final String ASCENDING = "ascending";
private static final int DEFAULT_ROW_LIMIT = 500;
private final MetricRegistry metrics;

private Timer.Context markAndTime(String subject) {
Expand Down Expand Up @@ -117,7 +118,7 @@ protected WaterSupplyAccountingDao getWaterSupplyAccountingDao(DSLContext dsl) {
@OpenApiParam(name = ASCENDING, description = "Whether or not the entries should be returned "
+ "in ascending order. Defaults to TRUE.", type = Boolean.class),
@OpenApiParam(name = ROW_LIMIT, description = "The maximum number of rows to return. "

@rma-bryson rma-bryson Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to use -1 for "no limit" ?

@MikeNeilson MikeNeilson Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's what's used for time series.

@RyanM-RMA RyanM-RMA Oct 20, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We were modelling this behavior off the way turbines and gate changes work. Negative row limit is used to say how many rows before the end date, vs positive row limit where it's how many rows after the start date.

@rma-psmorris rma-psmorris Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Gates, turbines, and water supply treat the row limit as follows:

  • negative values limit rows off of head
  • positive values limit rows off of tail
  • to not have a row limit, Integer.MAX_VALUE is specified (yes, this is an annoyingly large value).

@rma-psmorris rma-psmorris Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@MikeNeilson @rma-bryson -- do you have any additional feedback on this PR or can it be approved?

@MikeNeilson MikeNeilson Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we want to keep the head/tail behavior, I suggest we create additional parameters.

+ "Defaults to 0, which means no limit.", type = Integer.class)
+ "Defaults to " + DEFAULT_ROW_LIMIT + ".", type = Integer.class)
},
pathParams = {
@OpenApiParam(name = OFFICE, description = "The office ID of the project the "
Expand Down Expand Up @@ -165,7 +166,15 @@ public void handle(Context ctx) {
.getOrDefault(true);
final boolean endInclusive = ctx.queryParamAsClass(END_TIME_INCLUSIVE, Boolean.class).getOrDefault(true);
final boolean ascending = ctx.queryParamAsClass(ASCENDING, Boolean.class).getOrDefault(true);
final int rowLimit = ctx.queryParamAsClass(ROW_LIMIT, Integer.class).getOrDefault(0);
final int rowLimit = ctx.queryParamAsClass(ROW_LIMIT, Integer.class).getOrDefault(DEFAULT_ROW_LIMIT);

if (rowLimit == 0) {
CdaError error = new CdaError("Unable to retrieve accounting - " + ROW_LIMIT + " must be greater than 0.");
LOGGER.atSevere().log("Unable to retrieve accounting - " + ROW_LIMIT + " must be greater than 0.");
ctx.status(HttpServletResponse.SC_BAD_REQUEST).json(error);
return;
}

DSLContext dsl = getDslContext(ctx);

String formatHeader = ctx.headerAsClass(Header.ACCEPT, String.class).getOrDefault(Formats.JSONV1);
Expand Down
71 changes: 71 additions & 0 deletions cwms-data-api/src/test/java/cwms/cda/api/WaterSupplyAccountingControllerIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,77 @@ void testStoreNonExistentTransferType() throws Exception {
.assertThat()
.statusCode(is(HttpServletResponse.SC_BAD_REQUEST))
;

given()
.log().ifValidationFails(LogDetail.ALL, true)
.contentType(Formats.JSONV1)
.header(AUTH_HEADER, user.toHeaderValue())
.queryParam(START_TIME, "2005-04-05T00:00:00Z")
.queryParam(END_TIME, "2335-04-06T00:00:00Z")
.queryParam(START_INCLUSIVE, "true")
.queryParam(END_INCLUSIVE, "true")
.queryParam(ASCENDING, "true")
.queryParam(ROW_LIMIT, 100)
.queryParam(UNIT, "cfs")
.when()
.redirects().follow(true)
.redirects().max(3)
.get("/projects/" + OFFICE_ID + "/" + contract.getWaterUser().getProjectId().getName() + "/water-user/"
+ contract.getWaterUser().getEntityName() + "/contracts/"
+ contract.getContractId().getName() + "/accounting")
.then()
.log().ifValidationFails(LogDetail.ALL, true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_OK));
}

@Test
void testGet0RowLimit() throws Exception {
// 1 Store data
// 2 Retrieve accounting with 0 row limit
// 3 Verify failure from row limit
TestAccounts.KeyUser user = TestAccounts.KeyUser.SPK_NORMAL;
String json = JsonV1.buildObjectMapper().writeValueAsString(waterSupplyAccounting);

// create pump accounting
given()
.log().ifValidationFails(LogDetail.ALL, true)
.contentType(Formats.JSONV1)
.body(json)
.header(AUTH_HEADER, user.toHeaderValue())
.when()
.redirects().follow(true)
.redirects().max(3)
.post("/projects/" + OFFICE_ID + "/" + contract.getWaterUser().getProjectId().getName() + "/water-user/"
+ contract.getWaterUser().getEntityName() + "/contracts/"
+ contract.getContractId().getName() + "/accounting")
.then()
.log().ifValidationFails(LogDetail.ALL, true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_CREATED))
;

given()
.log().ifValidationFails(LogDetail.ALL, true)
.contentType(Formats.JSONV1)
.header(AUTH_HEADER, user.toHeaderValue())
.queryParam(START_TIME, "2005-04-05T00:00:00Z")
.queryParam(END_TIME, "2335-04-06T00:00:00Z")
.queryParam(START_INCLUSIVE, "true")
.queryParam(END_INCLUSIVE, "true")
.queryParam(ASCENDING, "true")
.queryParam(ROW_LIMIT, 0)
.queryParam(UNIT, "cfs")
.when()
.redirects().follow(true)
.redirects().max(3)
.get("/projects/" + OFFICE_ID + "/" + contract.getWaterUser().getProjectId().getName() + "/water-user/"
+ contract.getWaterUser().getEntityName() + "/contracts/"
+ contract.getContractId().getName() + "/accounting")
.then()
.log().ifValidationFails(LogDetail.ALL, true)
.assertThat()
.statusCode(is(HttpServletResponse.SC_BAD_REQUEST));
}

private static Location buildTestLocation(String name, String locationKind) {
Expand Down
Loading