Unit params tets #1512

maigbao · 2025-08-02T03:24:02Z

Description

Implemented strict schema-based validation for /addUnit and /edit API routes, ensuring only allowed parameters are accepted and unknown or malformed fields are rejected. Enforced field-level validation rules such as string lengths, enum values, numerical ranges, and boolean checks. Added additionalProperties: false and maxProperties to guard against extra fields in client payloads. Integrated a reusable validation test suite using helper functions like validateString, validateInt, validateBool, and validateMinMaxRelation to cover all supported data types and business logic (e.g., minVal <= maxVal constraint). Strengthened test coverage with unit tests to detect unexpected or invalid parameters during request handling.

Partly Addresses #1497

Type of change

(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])

Note merging this changes the database configuration.
This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

I have followed the OED pull request ideas
I have removed text in ( ) from the issue request
You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

Need to review

These probably are not perfect and there may be other thoughts later on.

huss

Thanks to @maigbao for this contribution. First, you checked the box in the description that you signed the CLA. OED records to not show your GitHub ID. Please do this or let me know if you think the records are incorrect. Second, I have made a number of comments about the code for you to consider. I note that running the code worked fine for what it intended. Please let me know if you want any help or have thoughts.

src/server/routes/units.js

src/server/test/util/vaidationHelpers.js

src/server/util/readingsUtils.js

src/server/test/util/unitTestUtils.js

src/server/test/routes/unitAddParamsTest.js

omarraf · 2025-08-14T21:50:02Z

Hello! I'm currently working on the remaining routes using this testing approach and I was just curious why there are two separate test files for units (unitAddParamsTest.js and unitEditParamsTest.js)? Thanks in advance!

huss · 2025-08-14T22:05:32Z

@maigbao can likely address better since they developed the software. I believe it was a separation of the different types of tests. It is true that many create (add) and edit routes are similar but they do differ some. If there was an easy way to integrate that would be nice/fine.

…nor formatting errors

maigbao · 2025-08-18T19:49:58Z

@omarraf They're separated because add and edit routes have different validation needs. Adding needs the complete fields, so tests test complete payloads and rigid rules. Editing allows partial updates, so tests aim at optional fields and legitimate updates. By separating them, suites are more readable, easier to maintain, and allow for quicker identification of which route a failure applies to.

huss · 2025-08-24T20:52:36Z

I pushed a commit to address some unresolved issues from comments.

huss

@maigbao Thank you for the update. I've made a few new comments. I also pushed a commit to address a few items that were unresolved but easier for me to fix than leave. I also unresolved a few comments as I could not determine they were done. There are also a number of comment that were unresolved from past comments. I'm going to ask you not to resolve any comments but put a reply instead so I can track the status and check myself. I will resolve as desired. Please reply on each unresolved comment that it is done, a reason it is not done or your thoughts. Once that is done you can leave a general comment that it is ready for another pass. Please let me know if you have any questions.

src/server/test/util/vaidationHelpers.js

src/server/test/routes/unitAddParamsTest.js

src/server/test/routes/unitEditParamsTest.js

huss

Thanks to @maigbao for the updated code. There is one previous comment that is still unresolved that I commented on. I also made a few new comments. Finally, the tests in src/server/test/routes/unitEditParamsTest.js are failing with a timeout and I also see Promise rejection was handled asynchronously. I did not look into this but something that should be fixed. Please let me know if anything is not clear or you need help.

src/server/test/routes/unitAddParamsTest.js

src/server/test/routes/unitEditParamsTest.js

src/server/test/util/validationHelpers.js

src/server/test/util/vaidationHelpers.js

huss · 2025-09-07T19:39:32Z

src/server/sql/baseline/create_baseline_table.sql

 CREATE TABLE IF NOT EXISTS baseline (
-	meter_id INT NOT NULL REFERENCES meters (id),
-	apply_range tsrange NOT NULL,
+    meter_id INT NOT NULL REFERENCES meters (id),


This file is showing changes due to spacing. Please remove them all since there are not real changes as part of this PR.

I copied and pasted the last version of this file because it said I changed this file in my pull request

It appears you changed white spacing in the file so it still shows up as having changing in the PR. Please fix this and verify it is correct via git and/or changes listed in the PR.

src/server/util/readingsUtils.js

huss

Thanks to @maigbao for the updated code. It resolved most of the previous comments and I made some new ones.

The tests for src/server/test/routes/unitAddParamsTest.js are all failing. This happens locally for me and similarly shown on GitHub in the PR. Each test has something similar to:

(node:145) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)
(Use `node --trace-warnings ...` to show where the warning was created)
    1) should accept a valid payload

and

 1) Validation - /addUnit
       should accept a valid payload:
     Error: Timeout of 15000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/usr/src/app/src/server/test/routes/unitAddParamsTest.js)
      at listOnTimeout (node:internal/timers:569:17)
      at process.processTimers (node:internal/timers:512:7)

I have not looked into why but it needs to be resolved. Please verify that the tests pass locally and on GitHub.

huss · 2025-09-21T12:37:38Z

src/server/test/routes/unitAddParamsTest.js

+			required: false
+		});
+		await validateString({
+			field: 'note',


I noticed that note does not have a maxLength check as done in edit. I think it should. This also indicates that the GLOBAL_STRING_MAX should be moved to a central file and exported for use across tests. Maybe in src/server/util in a new file called routeTesting.js. That would be somewhat similar to src/server/util/determineMaxPoints.js with a function for its values (but it does not need to be a function in the new case).

yes, I will separate it in one file

huss · 2025-09-21T12:42:12Z

src/server/routes/units.js

 const { success, failure } = require('./response');
 const router = express.Router();

+const GLOBAL_STRING_MAX = 1024;


First, this does not seem to be used in this file so why is it here? I suspect you meant it to be NOTE_MAX_LENGTH. If so, I'm unclear on why it should differ from the global string max. I think it is likely that this should use the same constant so see comment in unitAddParamsTest.

yes, it seems like I didn't commit the latest version of code

huss · 2025-09-21T12:43:57Z

src/server/routes/units.js

 			note: {
 				oneOf: [
-					{ type: 'string' },
+					{ type: 'string', maxLength: NOTE_MAX_LENGTH },


NOTE_MAX_LENGTH is not defined. See comment above.

huss · 2025-09-21T12:48:47Z

src/server/routes/units.js

+	validatorResult.errors = minMaxCheck.errors;
+
 	if (!validatorResult.valid) {
 		log.warn(`Got request to edit units with invalid unit data, errors: ${validatorResult.errors}`);


I think what is logged should match what is returned to the user. Please make the entire string a new const variable and then reuse it in both the log.warn and failure. There are growing examples of this usage pattern in the code.

src/server/test/routes/unitEditParamsTest.js

huss · 2025-09-21T12:55:57Z

src/server/test/util/validationHelpers.js

+ * - `minLength` and `maxLength` default to -1, which means "no check".
+ * - If `minLength = 0`, the function ensures the empty string is invalid.
+ * - If `maxLength = 0`, only an empty string would be allowed.
+ * @param field       the name of the string field to validate


OED generally does not like using spaces for indentation. In this case there should be no extra spaces since JSDoc formats what is shown so they are not rendered. I don't think there is a need to line up the descriptions with spacing but tabs should be used if it was done. I advocate for removing them.

huss · 2025-09-21T12:59:43Z

src/server/test/util/validationHelpers.js

+ * @param maxLength   the maximum length allowed for the string (default: -1 → no check)
+ * @param enumValues  optional array of valid enum values to test against
+ */
+ async function validateString({field, endpoint, basePayload, required = true, minLength = -1, maxLength = -1, enumValues = null}) {


You removed the space after { and before }. This is not done anywhere else in the file. I recommend you use VSC format document command to format your files.

huss · 2025-09-21T13:04:22Z

src/server/sql/baseline/create_baseline_table.sql

 CREATE TABLE IF NOT EXISTS baseline (
-	meter_id INT NOT NULL REFERENCES meters (id),
-	apply_range tsrange NOT NULL,
+    meter_id INT NOT NULL REFERENCES meters (id),


It appears you changed white spacing in the file so it still shows up as having changing in the PR. Please fix this and verify it is correct via git and/or changes listed in the PR.

huss · 2025-11-19T21:46:55Z

@maigbao Given that PR #1528 is continuing to move forward, I wanted to touch base to see the status of this PR. It is fine if you want to work on it but it is also fine to let us know that you would like someone else to finish it up. Thanks.

Bao Mai and others added 9 commits May 8, 2025 20:03

Update task 1, work on tesing POST unit endpoint

1cded97

Update for review

9b5a6f1

Update for review

7f5665b

Update unitTestUltils and relevant file

048013e

Update forgotten file

39038b6

update for review

074c163

Steve comments

9b60727

These probably are not perfect and there may be other thoughts later on.

Finish unit params test - waiting for review

1ed4482

fixing header

0c5d804

huss requested changes Aug 3, 2025

View reviewed changes

huss mentioned this pull request Aug 7, 2025

systematic parameter route testing #1497

Open

Bao Mai added 2 commits August 9, 2025 11:18

Small update in routes/units.js

dd9c8b8

Merge development

bc60ea0

Fixed the authentication for unit add and edit params test & fixed mi…

ab3c31c

…nor formatting errors

omarraf mentioned this pull request Aug 19, 2025

Implement systematic parameter validation testing across all server routes #1528

Open

5 tasks

fix formatting, fix file name & a couple of comments

2411226

huss requested changes Aug 24, 2025

View reviewed changes

src/server/test/util/vaidationHelpers.js Show resolved Hide resolved

src/server/test/util/vaidationHelpers.js Show resolved Hide resolved

src/server/test/routes/unitAddParamsTest.js Outdated Show resolved Hide resolved

src/server/test/routes/unitEditParamsTest.js Show resolved Hide resolved

Bao Mai added 3 commits August 31, 2025 22:42

Fixing following the comments

e9ca210

Generalize min max relation in units.js route

90812ef

redo to old version of the file

399d313

huss requested changes Sep 7, 2025

View reviewed changes

Bao Mai added 5 commits September 7, 2025 16:11

Fix unitEdit

173f062

Delete function

b9e89d9

remove extra space

0444cfb

Fix validateString

0df94e1

Global limit for note

c019693

huss requested changes Sep 21, 2025

View reviewed changes

Bao Mai added 2 commits October 5, 2025 14:46

Fix based on suggestion

4bccca0

Revert file

7a8bb9c

Unit params tets #1512

Are you sure you want to change the base?

Unit params tets #1512

Uh oh!

Conversation

maigbao commented Aug 2, 2025

Description

Type of change

Checklist

Limitations

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

omarraf commented Aug 14, 2025

Uh oh!

huss commented Aug 14, 2025

Uh oh!

maigbao commented Aug 18, 2025

Uh oh!

huss commented Aug 24, 2025

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

huss 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!

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