Fixes #29494: NPE in ServiceEntityResource.unmask when request has no connection - #29497
Fixes #29494: NPE in ServiceEntityResource.unmask when request has no connection#29497ivnvMkhl wants to merge 4 commits into
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
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! |
TeddyCr
added
the
safe to test
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
Jest test CoverageUI tests summary
|
🔴 Playwright Results — 8 test failure(s), 4 pipeline/setup failure(s)✅ 2883 passed · ❌ 8 failed · 🟡 11 flaky · ⏭️ 82 skipped Pipeline and setup failures
Genuine Failures (failed on all attempts)❌
|
|
|
…equest has no connection unmask() guarded against originalService.getConnection() == null but not against service.getConnection() == null (the incoming request). A PUT with no connection field (idempotent ensure-service-exists) threw NullPointerException when the service already existed. Added the missing null check.
8491f16 to
de22c83
Compare
|
@ivnvMkhl please add a integration test to reproduce this issue |
Per review feedback (harshach): put_updateWithoutConnection_preservesExistingConnection_200 drives a raw PUT /v1/services/databaseServices with no connection field against a service that already has one, hitting ServiceEntityResource.unmask() directly. Fails with a 500 (NPE) without the null-check fix; asserts a 200 and that the pre-existing connection is left untouched. Also adds put_updateWithoutConnection_whenNoExistingConnection_200 as a smoke check on the neighboring branch (no connection on either side), which was already guarded before this fix.
|
@harshach Added an integration test — |
Code Review ✅ ApprovedPrevents a NullPointerException in ServiceEntityResource by adding a guard for missing connection fields in idempotent PUT requests. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
This PR has had no activity for 30 days and will be closed in 7 days if no further activity occurs. |
3 participants
Description of Changes
Fixes #29494
ServiceEntityResource.unmask()checkedoriginalService.getConnection() != null(the entity stored in DB) before callingunmaskServiceConnectionConfig, but did not checkservice.getConnection() != null(the incoming request body). When aPUT /api/v1/services/databaseServicesrequest omits theconnectionfield — a valid idempotent upsert pattern — the server threw aNullPointerException:The fix adds the missing
&& service.getConnection() != nullguard. When the incoming request has no connection, the unmask block is skipped and the existing connection in the DB is preserved unchanged.Type of Change
High-level Design
N/A — single-condition null check addition.
Testing Coverage
PUT /api/v1/services/databaseServiceswith{"name": "svc", "serviceType": "Hive"}on an existing service. Confirmed 200 response after the fix.UI Evidence
Not applicable.
Checklist
Fixes #<issue>: <description>formatSummary by Gitar
NullPointerExceptioninServiceEntityResourceduring idempotentPUTrequests when theconnectionfield is omitted.service.getConnection() != nullguard to safely handle requests without connection details.This will update automatically on new commits.
Greptile Summary
This PR fixes a
NullPointerExceptioninServiceEntityResource.unmask()that occurred when aPUT /api/v1/services/databaseServicesrequest omitted theconnectionfield. The fix adds aservice.getConnection() != nullguard alongside the existingoriginalService.getConnection() != nullcheck, so the unmask block is only entered when both sides have a connection.ServiceEntityResource.java: Adds the missing null guard on the incomingservice.getConnection()inunmask(), preventing an NPE when the request body omitsconnection.DatabaseServiceResourceIT.java: Adds two integration tests covering the fixed scenarios — PUT without connection on a service that has one (verifies existing connection is preserved), and PUT without connection on a service that never had one (verifies no NPE).Confidence Score: 5/5
Safe to merge — the change is a single, targeted null guard on a code path that had no protection for a null incoming connection.
The fix is minimal (one additional boolean condition), clearly correct, and directly addresses the root cause. Both code paths — a service that has an existing connection and one that never had a connection — are exercised by the new integration tests. No other behavior is altered.
No files require special attention.
Important Files Changed
service.getConnection() != nullguard tounmask()— minimal, correct null check that prevents NPE on PUT requests with no connection.Sequence Diagram
sequenceDiagram participant Client participant ServiceEntityResource participant Repository participant EntityMasker Client->>ServiceEntityResource: PUT /services/databaseServices (no connection field) ServiceEntityResource->>Repository: findByNameOrNull(fqn) Repository-->>ServiceEntityResource: originalService (with connection) Note over ServiceEntityResource: Before fix ServiceEntityResource->>ServiceEntityResource: "if originalService != null && originalService.getConnection() != null" ServiceEntityResource->>EntityMasker: unmaskServiceConnectionConfig(service.getConnection().getConfig(), ...) Note over ServiceEntityResource: service.getConnection() is null → NPE 💥 Note over ServiceEntityResource: After fix ServiceEntityResource->>ServiceEntityResource: "if originalService != null && originalService.getConnection() != null && service.getConnection() != null" Note over ServiceEntityResource: guard fails → block skipped ✅ ServiceEntityResource-->>Client: 200 OKReviews (6): Last reviewed commit: "Merge branch 'main' into fix/service-unm..." | Re-trigger Greptile