Skip to content

Conversation

@dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Aug 22, 2025

Description

The ML Client requires a few runtime dependencies that are not included in the shadow JAR, primarily associated with validation of ML Connector fields. The requirement to have these dependencies by downstream consumers of the client is not documented nor obvious.

In the Maven dependency ecosystem, the provided dependency scope is a clear indicator of a required runtime dependency that the project is not providing itself:

provided
This is much like compile, but indicates you expect the JDK or a container to provide the dependency at runtime. For example, when building a web application for the Java Enterprise Edition, you would set the dependency on the Servlet API and related Java EE APIs to scope provided because the web container provides those classes. A dependency with this scope is added to the classpath used for compilation and test, but not the runtime classpath. It is not transitive.

This PR appends a <dependency> section to the published opensearch-ml-client POM file on the Central Repository. The additional section added will be:

<dependencies>
  <dependency>
    <groupId>org.json</groupId>
    <artifactId>json</artifactId>
    <version>20231013</version>
    <scope>provided</scope>
  </dependency>
  <dependency>
    <groupId>com.jayway.jsonpath</groupId>
    <artifactId>json-path</artifactId>
    <version>2.9.0</version>
    <scope>provided</scope>
  </dependency>
  <dependency>
    <groupId>org.apache.commons</groupId>
    <artifactId>commons-text</artifactId>
    <version>1.10.0</version>
    <scope>provided</scope>
  </dependency>
</dependencies>

Note that there is an additional dependency in StringUtils on com.networknt:json-schema-validator:1.4.0. Currently this is not required by the ML Client, but if any future changes to the client include schema validation, that should be added to this list.

This has no impact on actual included dependencies; it's simply a documentation signal to clearly indicate to consumers of this client that they must provide these dependencies themselves.

Related Issues

Check List

  • New functionality has been documented.
  • 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.

Summary by CodeRabbit

  • Chores
    • Updated build configuration to specify external library dependencies that must be provided by the runtime environment rather than bundled with the application.

✏️ Tip: You can customize this high-level summary in your review settings.

@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:06 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:06 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:06 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:06 — with GitHub Actions Failure
@dbwiddis dbwiddis force-pushed the client-dependencies branch from 8ef79c2 to 3c8413b Compare August 22, 2025 22:10
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:12 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:12 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:12 — with GitHub Actions Error
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 22, 2025 22:12 — with GitHub Actions Failure
withXml {
def dependencies = asNode().appendNode('dependencies')

def addProvidedDependency = { group, artifact, version ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing this issue. Have you verified this? Any impact to other plugins using ml-commons clients like neural-search?

Copy link
Member Author

Choose a reason for hiding this comment

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

This really only changes the POM which wouuld in theory add transitive dependencies, ecxept:

  • we don't do transitive dependencies in OpenSearch (the source of the problem!)
  • it's in scope <provided> which means it wouldn't be imported transitively if it did. This is purely a user documentation change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did test it with flow framework by deleting those imports (and getting runtime errors) even with the new snapshot containing the new values in the pom in my maven local repository, so it confirms these imports aren't included.

@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 29, 2025 01:22 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 29, 2025 01:22 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 29, 2025 01:22 — with GitHub Actions Failure
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval August 29, 2025 01:22 — with GitHub Actions Failure
@coderabbitai
Copy link

coderabbitai bot commented Dec 11, 2025

Walkthrough

Adds three dependencies with provided scope to the Gradle shadow publication POM configuration: org.json, json-path, and commons-text. These dependencies are marked as provided to indicate they must be supplied by the consuming runtime environment rather than bundled with the client artifact.

Changes

Cohort / File(s) Summary
Build Dependency Configuration
client/build.gradle
Adds withXml block to shadow publication introducing three provided-scope dependencies: org.json:json:20231013, com.jayway.jsonpath:json-path:2.9.0, and org.apache.commons:commons-text:1.10.0

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Verify dependency versions are correct and compatible with the project's Java/library versions
  • Confirm that marking these as provided scope is the appropriate resolution for the classloader visibility issue described in the linked issue

Poem

🐰 A rabbit builds bridges, both swift and sublime,
With dependencies marked as "provided" in time,
No bundling or clashing in classloader's sight,
Each consumer will find what they need, all just right! 🛠️

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add provided dependency section to ml-client POM' clearly and accurately describes the main change - adding a dependencies section with provided scope to the POM file.
Description check ✅ Passed The description provides comprehensive context, explains the rationale for the change, documents the dependencies being added, and completes most checklist items appropriately.
Linked Issues check ✅ Passed The PR addresses issue #3860 by documenting required runtime dependencies as 'provided' scope in the POM, which clearly signals to downstream consumers that they must supply these dependencies themselves.
Out of Scope Changes check ✅ Passed All changes are scoped to adding the dependencies section to the POM file as required by issue #3860; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6de6889 and f8ab4b7.

📒 Files selected for processing (1)
  • client/build.gradle (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Mend Security Check
  • GitHub Check: spotless
🔇 Additional comments (1)
client/build.gradle (1)

111-126: Well-executed POM dependencies declaration—correctly solves the transitive dependency visibility issue.

The implementation cleanly declares runtime dependencies as "provided" in the published POM, signaling downstream consumers that they must supply org.json, json-path, and commons-text. The use of a closure pattern to reduce repetition is idiomatic and maintainable.

This approach properly addresses issue #3860 by documenting dependencies that StringUtils requires during request validation but aren't bundled in the shadow JAR. Per prior reviews, this has been tested with flow framework and is purely documentation-focused with no impact on the actual artifact contents.

One minor note for clarity: the PR objectives mention json-schema-validator (1.4.0) is also used by StringUtils but intentionally omitted here until schema validation is actually introduced. The current three dependencies cover the present runtime requirements.


Comment @coderabbitai help to get the list of available commands and usage tips.

@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env-require-approval December 11, 2025 00:07 — with GitHub Actions Inactive
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval December 11, 2025 00:07 — with GitHub Actions Error
@dbwiddis dbwiddis temporarily deployed to ml-commons-cicd-env-require-approval December 11, 2025 00:07 — with GitHub Actions Inactive
@dbwiddis dbwiddis had a problem deploying to ml-commons-cicd-env-require-approval December 11, 2025 00:07 — with GitHub Actions Failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Downstream dependencies struggle to find transitive dependencies in StringUtils

2 participants