add validation for name and description for model model group and connector resources #3805

dhrubo-os · 2025-04-29T19:29:42Z

add validation for name and description for model model group and connector resources

Description

resolved #3639

Integ test will fail as code coverages is failing for previously merged PR.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

ylwu-amzn · 2025-04-29T19:36:52Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorRequest.java

+
+        if (modelName != null && !isSafeText(modelName)) {
+            exception = addValidationError(
+                "Model connector name can only contain letters, digits, spaces, underscores (_), hyphens (-), and dots (.)",


This error message is different with MLUpdateConnectorRequest.java one. Should we keep them consistent ?

"Model connector name can only contain letters, digits, spaces, underscores (_), hyphens (-), and dots (.). Max length: 1000 characters.",

Yes, in the next revision I kept it consistent.

ylwu-amzn · 2025-04-29T19:37:48Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLUpdateConnectorRequest.java

+
+            if (modelName != null && !isSafeText(modelName)) {
+                exception = addValidationError(
+                    "Model connector name can only contain letters, digits, spaces, underscores (_), hyphens (-), and dots (.). Max length: 1000 characters.",


Don't see where validate Max length: 1000 characters." , Is that built-in logic in isSafeText?

My bad, I removed that restriction from the regex. But then I forgot to remove that from the error message.

Initially I was thinking to provide 1000 characters restriction.

What do you think? Should we add such kind of restriction?

Ignore the comment, find that internal logic in isSafeText

ylwu-amzn · 2025-04-29T19:39:23Z

common/src/main/java/org/opensearch/ml/common/transport/model/MLUpdateModelRequest.java

+        String modelName = updateModelInput.getName();
+        String description = updateModelInput.getDescription();
+
+        if (modelName != null && !isSafeText(modelName)) {


Similar checking in multiple places ? Can we build some common util method ?

Yeah, I thought about that and felt too lazy to refactor 😄. But I fixed it in the second revision. Thanks for insisting on the highest standards!

…nector resources Signed-off-by: Dhrubo Saha <[email protected]>

…undnancy Signed-off-by: Dhrubo Saha <[email protected]>

Signed-off-by: Dhrubo Saha <[email protected]>

Zhangxunmt · 2025-05-07T19:06:55Z

common/src/main/java/org/opensearch/ml/common/utils/StringUtils.java

+
+            if (value != null && !isSafeText(value)) {
+                exception = addValidationError(
+                    key + " can only contain letters, digits, spaces, underscores (_), hyphens (-), dots (.), and colons (:)",


To improve the performance, using StringBuilder msgBuilder = new StringBuilder() for error messages will reduce the memory usage.

Zhangxunmt · 2025-05-07T19:11:40Z

common/src/main/java/org/opensearch/ml/common/utils/StringUtils.java

+        if (value == null || value.isBlank()) {
+            return false;
+        }
+        return value.matches(SAFE_INPUT_REGEX);


Pattern SAFE_TEXT_PATTERN = Pattern.compile(SAFE_INPUT_REGEX); Using Pattern matcher is more efficient as it only Compile pattern once rather than Compiles pattern every time in String.matches():

rithin-pullela-aws · 2025-05-07T20:03:29Z

common/src/main/java/org/opensearch/ml/common/utils/StringUtils.java

@@ -60,6 +63,9 @@ public class StringUtils {
        + "      return input;"
        + "\n    }\n";

+    // Regex allows letters, digits, spaces, hyphens, underscores, and dots.
+    private static final String SAFE_INPUT_REGEX = "^[a-zA-Z0-9 _\\-\\.:,'()]+$";


Just curious how we determined the regex is this a standard regex generally used? I believe we want to prevent <, >, ;, ", /, \, =.
Since we are using this for description, maybe we need to allow !, @ etc?

dhrubo-os requested review from b4sjoo, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and xinyual as code owners April 29, 2025 19:29

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 19:31 — with GitHub Actions Failure

ylwu-amzn reviewed Apr 29, 2025

View reviewed changes

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:45 — with GitHub Actions Failure

dhrubo-os force-pushed the input_validation branch from 0326043 to d8eeede Compare April 29, 2025 20:49

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:50 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:50 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 20:50 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 23:25 — with GitHub Actions Failure

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 23:25 — with GitHub Actions Error

dhrubo-os had a problem deploying to ml-commons-cicd-env April 29, 2025 23:25 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env April 30, 2025 19:56 — with GitHub Actions Inactive

dhrubo-os added 4 commits April 30, 2025 16:39

add validation for name and description for model model group and con…

a6923be

…nector resources Signed-off-by: Dhrubo Saha <[email protected]>

addressing comment, added a common method in utils to remove code red…

989edf5

…undnancy Signed-off-by: Dhrubo Saha <[email protected]>

making more flexible the regex pattern

3f0f81a

Signed-off-by: Dhrubo Saha <[email protected]>

removing restriction of 1000 characters

460af00

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os force-pushed the input_validation branch from ae85e9d to 460af00 Compare April 30, 2025 20:42

dhrubo-os temporarily deployed to ml-commons-cicd-env April 30, 2025 20:43 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env April 30, 2025 21:47 — with GitHub Actions Inactive

Merge branch 'main' into input_validation

5dfb719

dhrubo-os temporarily deployed to ml-commons-cicd-env May 6, 2025 23:40 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env May 7, 2025 00:43 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 00:43 — with GitHub Actions Inactive

dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 03:12 — with GitHub Actions Inactive

Zhangxunmt reviewed May 7, 2025

View reviewed changes

Merge branch 'main' into input_validation

2bed532

dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 19:15 — with GitHub Actions Inactive

rithin-pullela-aws reviewed May 7, 2025

View reviewed changes

dhrubo-os temporarily deployed to ml-commons-cicd-env May 7, 2025 20:17 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add validation for name and description for model model group and connector resources #3805

add validation for name and description for model model group and connector resources #3805

dhrubo-os commented Apr 29, 2025 •

edited

Loading

ylwu-amzn Apr 29, 2025 •

edited

Loading

dhrubo-os Apr 29, 2025

ylwu-amzn Apr 29, 2025

dhrubo-os Apr 29, 2025

ylwu-amzn Apr 29, 2025

ylwu-amzn Apr 29, 2025

dhrubo-os Apr 29, 2025

Zhangxunmt May 7, 2025

Zhangxunmt May 7, 2025

rithin-pullela-aws May 7, 2025

add validation for name and description for model model group and connector resources #3805

Are you sure you want to change the base?

add validation for name and description for model model group and connector resources #3805

Conversation

dhrubo-os commented Apr 29, 2025 • edited Loading

Description

Related Issues

Check List

ylwu-amzn Apr 29, 2025 • edited Loading

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

dhrubo-os commented Apr 29, 2025 •

edited

Loading

ylwu-amzn Apr 29, 2025 •

edited

Loading