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

Jump to bottom

add parameter validate for PPL tool #259

Merged
merged 8 commits into from
Mar 18, 2024
Merged

add parameter validate for PPL tool #259

merged 8 commits into from
Mar 18, 2024

Conversation

xinyual
Copy link
Collaborator

@xinyual xinyual commented Mar 14, 2024

Description

add parameter validate for PPL tool:

  1. Model id should not be null or blank
  2. Head should be integer
  3. Execute should be false or true

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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.

@xinyual
apply spotless
47b8482 
Signed-off-by: xinyual <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Mar 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 81.62%. Comparing base (cbe5fa2) to head (739c680).
Report is 2 commits behind head on main.

Files Patch % Lines
.../main/java/org/opensearch/agent/tools/PPLTool.java 85.71% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main     #259      +/-   ##
============================================
+ Coverage     81.57%   81.62%   +0.05%     
- Complexity      211      216       +5     
============================================
  Files            13       13              
  Lines          1069     1083      +14     
  Branches        146      149       +3     
============================================
+ Hits            872      884      +12     
  Misses          140      140              
- Partials         57       59       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xinyual
simplify code
b2da40e 
Signed-off-by: xinyual <[email protected]>
zane-neo
zane-neo reviewed Mar 14, 2024
View reviewed changes
src/main/java/org/opensearch/agent/tools/PPLTool.java
throw new IllegalArgumentException("PPL tool needs non blank model id.");
}
if (map.containsKey("execute")) {
String execute = map.get("execute").toString().toLowerCase(Locale.ROOT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible the map.get("execute") is null? if yes, it could produce NPE here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I change the logic to prevent it. Please check it. From my perspective, these parameters are from register body and couldn't be null. But anyway, it's better to have it.

@xinyual
prevent NPE
7ad3b4b 
Signed-off-by: xinyual <[email protected]>
xinyual added 2 commits March 15, 2024 09:33
@xinyual
change logic
3fd1b24 
Signed-off-by: xinyual <[email protected]>
@xinyual
apply spot
b8b1379 
Signed-off-by: xinyual <[email protected]>
xinyual added 2 commits March 18, 2024 15:31
@xinyual
fix UT
6c59da2 
Signed-off-by: xinyual <[email protected]>
@xinyual
apply spotless
739c680 
Signed-off-by: xinyual <[email protected]>
@zane-neo zane-neo merged commit 571dab7 into opensearch-project:main Mar 18, 2024
18 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 18, 2024
@github-actions
add parameter validate for PPL tool (#259)
533c581 
* add parameter validate for PPL tool

Signed-off-by: xinyual <[email protected]>

* apply spotless

Signed-off-by: xinyual <[email protected]>

* simplify code

Signed-off-by: xinyual <[email protected]>

* prevent NPE

Signed-off-by: xinyual <[email protected]>

* change logic

Signed-off-by: xinyual <[email protected]>

* apply spot

Signed-off-by: xinyual <[email protected]>

* fix UT

Signed-off-by: xinyual <[email protected]>

* apply spotless

Signed-off-by: xinyual <[email protected]>

---------

Signed-off-by: xinyual <[email protected]>
(cherry picked from commit 571dab7)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Mar 18, 2024
Merged
xinyual pushed a commit that referenced this pull request Mar 18, 2024
@opensearch-trigger-bot @github-actions
add parameter validate for PPL tool (#259) (#262)
64c5e2e 
* add parameter validate for PPL tool



* apply spotless



* simplify code



* prevent NPE



* change logic



* apply spot



* fix UT



* apply spotless



---------


(cherry picked from commit 571dab7)

Signed-off-by: xinyual <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
yuye-aws pushed a commit to yuye-aws/skills that referenced this pull request Apr 26, 2024
@opensearch-trigger-bot @github-actions @yuye-aws
add parameter validate for PPL tool (opensearch-project#259) (opensea…
2c06c2c 
…rch-project#262)

* add parameter validate for PPL tool

* apply spotless

* simplify code

* prevent NPE

* change logic

* apply spot

* fix UT

* apply spotless

---------

(cherry picked from commit 571dab7)

Signed-off-by: xinyual <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: yuye-aws <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zane-neo zane-neo zane-neo approved these changes

@b4sjoo b4sjoo Awaiting requested review from b4sjoo b4sjoo is a code owner

@dhrubo-os dhrubo-os Awaiting requested review from dhrubo-os dhrubo-os is a code owner

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@ylwu-amzn ylwu-amzn Awaiting requested review from ylwu-amzn ylwu-amzn is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@amitgalitz amitgalitz Awaiting requested review from amitgalitz amitgalitz is a code owner

@jackiehanyang jackiehanyang Awaiting requested review from jackiehanyang jackiehanyang is a code owner

@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

@ohltyler ohltyler Awaiting requested review from ohltyler ohltyler is a code owner

@joshpalis joshpalis Awaiting requested review from joshpalis joshpalis is a code owner

@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

@kaituo kaituo Awaiting requested review from kaituo kaituo is a code owner

@lezzago lezzago Awaiting requested review from lezzago lezzago is a code owner

@eirsep eirsep Awaiting requested review from eirsep eirsep is a code owner

@sbcd90 sbcd90 Awaiting requested review from sbcd90 sbcd90 is a code owner

@zhichao-aws zhichao-aws Awaiting requested review from zhichao-aws zhichao-aws is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

Assignees
No one assigned
Labels
backport 2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xinyual @zane-neo