Fixes #26869: Defensive type handling in AirflowRESTClient to prevent ClassCastException#27019
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
Pull request overview
Fixes ClassCastException during pipeline client initialization by making Airflow/K8s pipeline client parameter handling resilient to YAML-parsed numeric types and by adjusting default config values.
Changes:
- Updated
AirflowRESTClientto read parameters via safe string/int helpers instead of direct casts. - Added unit tests ensuring
timeoutparameter types (String,Integer,Long) don’t break client construction. - Updated example YAML configs to quote numeric defaults in
pipelineServiceClientConfiguration.parameters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
openmetadata-service/src/main/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClient.java |
Replaces fragile casts with getStringParam/getIntParam helpers for parameter parsing. |
openmetadata-service/src/test/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClientTest.java |
Adds constructor tests covering multiple numeric/string timeout representations. |
conf/openmetadata.yaml |
Quotes numeric defaults under pipelineServiceClientConfiguration.parameters. |
conf/openmetadata-s3-logs.yaml |
Quotes numeric defaults for Airflow timeout parameters in the S3 logs example config. |
conf/openmetadata.yaml
Outdated
| runAsUser: ${K8S_RUN_AS_USER:-"1000"} | ||
| runAsGroup: ${K8S_RUN_AS_GROUP:-"1000"} | ||
| fsGroup: ${K8S_FS_GROUP:-"1000"} |
There was a problem hiding this comment.
Same issue as above for securityContext IDs: quoting only the default doesn’t prevent numeric env var values from being parsed as numbers. Wrap the whole substitution in quotes (e.g. runAsUser: "${K8S_RUN_AS_USER:-1000}", etc.) so YAML always yields strings for parameters.*.
| runAsUser: ${K8S_RUN_AS_USER:-"1000"} | |
| runAsGroup: ${K8S_RUN_AS_GROUP:-"1000"} | |
| fsGroup: ${K8S_FS_GROUP:-"1000"} | |
| runAsUser: "${K8S_RUN_AS_USER:-1000}" | |
| runAsGroup: "${K8S_RUN_AS_GROUP:-1000}" | |
| fsGroup: "${K8S_FS_GROUP:-1000}" |
...rvice/src/main/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClient.java
Outdated
Show resolved
Hide resolved
RajdeepKushwaha5
force-pushed
the
fix/26869-yaml-integer-classcastexception
branch
from
7cfc3f7 to
d9d0d64
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
...rvice/src/main/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClient.java
Show resolved
Hide resolved
RajdeepKushwaha5
force-pushed
the
fix/26869-yaml-integer-classcastexception
branch
from
e62eef0 to
b9b7645
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
...rvice/src/main/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClient.java
Outdated
Show resolved
Hide resolved
RajdeepKushwaha5
force-pushed
the
fix/26869-yaml-integer-classcastexception
branch
from
b9b7645 to
b3b2c92
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
...rvice/src/main/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClient.java
Show resolved
Hide resolved
RajdeepKushwaha5
force-pushed
the
fix/26869-yaml-integer-classcastexception
branch
from
b3b2c92 to
7935d7a
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
RajdeepKushwaha5
changed the title
pmbrull
added
the
safe to test
🟡 Playwright Results — all passed (29 flaky)✅ 3595 passed · ❌ 0 failed · 🟡 29 flaky · ⏭️ 207 skipped
🟡 29 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
| Map<String, Object> params = | ||
| config.getParameters() != null | ||
| ? config.getParameters().getAdditionalProperties() | ||
| : Collections.emptyMap(); | ||
| this.username = getStringParam(params, USERNAME_KEY); | ||
| this.password = getStringParam(params, PASSWORD_KEY); |
There was a problem hiding this comment.
config.getParameters() is null-checked, but config.getParameters().getAdditionalProperties() can still be null in some generated config POJOs; in that case params becomes null and getStringParam(params, …) will throw a NPE before the intended credential validation. Consider defaulting to Collections.emptyMap() when getAdditionalProperties() is null as well (and add a unit test for this case).
| Map<String, Object> params = | ||
| config.getParameters() != null | ||
| ? config.getParameters().getAdditionalProperties() | ||
| : Collections.emptyMap(); | ||
| String truststorePath = getStringParam(params, TRUSTSTORE_PATH_KEY); | ||
| String truststorePassword = getStringParam(params, TRUSTSTORE_PASSWORD_KEY); | ||
|
|
There was a problem hiding this comment.
Same null-safety concern here: if config.getParameters() is non-null but getAdditionalProperties() is null, params will be null and getStringParam(params, …) will NPE. Prefer falling back to an empty map when getAdditionalProperties() is null.
...e/src/test/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClientTest.java
Show resolved
Hide resolved
Add null-safety check for getAdditionalProperties() in both the constructor and createAirflowSSLContext(). While the generated Parameters class initializes the field, deserialization could produce a null value. Falls back to emptyMap gracefully. Add constructorHandlesNullAdditionalProperties test to cover the case where Parameters is non-null but additionalProperties is null.
Code Review ✅ Approved 2 resolved / 2 findingsDefensive type handling in AirflowRESTClient now prevents ClassCastException and null pointer exceptions by adding validation to getIntParam and getAdditionalProperties(). Both issues have been resolved. ✅ 2 resolved✅ Quality: getIntParam silently swallows invalid config values
✅ Edge Case: getAdditionalProperties() can return null, causing NPE
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Describe your changes:
Fixes #26869
Problem
AirflowRESTClientconstructor performs direct(Integer)and(String)casts on values read fromconfig.getParameters().getAdditionalProperties(). When SnakeYAML parses unquoted numeric values liketimeout: 60inopenmetadata.yaml, it deserializes them asjava.lang.Integer. However, the code later casts these values with(String), causing aClassCastExceptionat runtime:This crashes the server on startup when pipeline service client parameters contain unquoted numeric YAML values.
Solution
Instead of fragile direct casts, this PR introduces two defensive helper methods in
AirflowRESTClient:getStringParam(params, key)— Retrieves a value and converts it toStringviavalue.toString(), safely handlingInteger,Long,String, or any other type SnakeYAML may produce.getIntParam(params, key, defaultValue)— Retrieves a value and converts it tointviaInteger.parseInt(value.toString()). Logs aLOG.warnwith the actual value if parsing fails, then falls back to the provided default.Additional improvements:
config.getParameters()andconfig.getParameters().getAdditionalProperties()usingCollections.emptyMap()fallback — prevents NPE whenParametersis present butadditionalPropertiesis null.PipelineServiceClientExceptionearly ifusernameorpasswordis null after parameter extraction.Testing
Added 7 new unit tests in
AirflowRESTClientTest:constructorHandlesStringTimeout"60"(String)constructorHandlesIntegerTimeout60(Integer)constructorHandlesLongTimeout60L(Long)constructorHandlesNullParametersconfig.getParameters()is nullconstructorHandlesNonNumericTimeout"abc"— exercises warn+fallback branchconstructorHandlesNullAdditionalPropertiesParametersis non-null butadditionalPropertiesis nullconstructorHandlesMissingCredentialsAll tests pass locally.
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>