Add integration tests for consent webhooks#28119
Conversation
📝 WalkthroughAdded end-to-end integration coverage for consent webhook events.
WalkthroughThis change adds integration test coverage for consent-related webhook events. A new Sequence Diagram(s)sequenceDiagram
participant TestCase as ConsentEventTestCase
participant ConsentClient as ConsentManagementRestClient
participant WebhookManager as WebhookEventTestManager
participant PayloadBuilder as ConsentEventTestExpectedEventPayloadBuilder
TestCase->>ConsentClient: createElement(), createPurpose()
ConsentClient-->>TestCase: elementId, purposeId
TestCase->>ConsentClient: createConsent(serviceId, purposeId, elementId, ...)
ConsentClient-->>TestCase: receiptId
TestCase->>PayloadBuilder: buildExpectedConsentAddedEventPayload(...)
PayloadBuilder-->>TestCase: expected payload
TestCase->>WebhookManager: stack and validate payload
TestCase->>ConsentClient: revokeConsent(receiptId, ...)
TestCase->>PayloadBuilder: buildExpectedConsentRevokedEventPayload(...)
PayloadBuilder-->>TestCase: expected payload
TestCase->>WebhookManager: stack and validate payload
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
PR builder started |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/ConsentManagementRestClient.java (2)
85-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo status assertion for delete/revoke operations.
deleteElement,deletePurpose, andrevokeConsentrely on comments (// expects 204,// expects 200) rather than actually asserting the status code. If these calls silently fail, subsequent test setup/teardown steps could proceed with stale state without a clear failure signal.Also applies to: 113-119, 153-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/ConsentManagementRestClient.java` around lines 85 - 91, The delete/revoke helpers in ConsentManagementRestClient are only documenting expected HTTP statuses instead of enforcing them, so update deleteElement, deletePurpose, and revokeConsent to assert the returned response status explicitly after getResponseOfHttpDelete/getResponseOfHttpPost. Use the existing response handling in these methods to check for 204 in the delete paths and 200 in revokeConsent, and fail fast if the status differs so test setup/teardown cannot continue with stale state.
100-108: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile string-replace for JSON substitution.
Substituting the element ID via
.replace("\"id\": \"1\"", ...)is brittle — it depends on exact whitespace/formatting increate-purpose.json. Any reformatting of that resource file (e.g., minification or different spacing) would silently break the substitution without a clear error, sinceString.replacereturns the original string unchanged if no match is found.Consider parsing and modifying the JSON via
ObjectMapper/ObjectNodeinstead for a more resilient substitution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/ConsentManagementRestClient.java` around lines 100 - 108, The createPurpose method uses a brittle String.replace on the create-purpose.json template to inject the ID, which depends on exact formatting. Update createPurpose to parse the resource with ObjectMapper, modify the JSON field directly through an ObjectNode, and then serialize the result before posting. Keep the existing getResponseOfHttpPost flow, but remove the hardcoded "\"id\": \"1\"" substitution so the code is resilient to whitespace or formatting changes in the resource.modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/webhooks/consent/ConsentEventTestCase.java (1)
130-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTeardown doesn't validate delete/revoke success and swallows partial failures.
In
atEnd(), ifdeletePurposeordeleteElementthrows (e.g., due to the client's lack of status validation noted inConsentManagementRestClient), the remaining cleanup steps (deleteElement,closeHttpClient,webhookEventTestManager.teardown()) will be skipped, potentially leaking test resources across test runs.Consider wrapping each cleanup step so failures don't prevent subsequent cleanup from running.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/webhooks/consent/ConsentEventTestCase.java` around lines 130 - 151, The teardown in atEnd() stops on the first cleanup failure, so later resources like closeHttpClient() and webhookEventTestManager.teardown() may be skipped. Update ConsentEventTestCase.atEnd() to wrap each cleanup call (deleteUser, deletePurpose, deleteElement, closeHttpClient, teardown) in its own guarded try/catch or equivalent so a failure in one step does not block the rest, and make sure the cleanup order still uses scim2RestClient, consentManagementRestClient, and webhookEventTestManager safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/ConsentManagementRestClient.java`:
- Around line 73-80: The createElement, createPurpose, and createConsent methods
in ConsentManagementRestClient parse the JSON body and call get("id").asText()
without first validating the HTTP response status. Add a status check
immediately after receiving the CloseableHttpResponse in each method, and only
parse the body when the response indicates success; otherwise fail with a clear
message that includes the HTTP status/body. Use the existing
getResponseOfHttpPost, readResponse, and ObjectMapper flow to keep the fix
consistent across all three methods.
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/ConsentManagementRestClient.java`:
- Around line 85-91: The delete/revoke helpers in ConsentManagementRestClient
are only documenting expected HTTP statuses instead of enforcing them, so update
deleteElement, deletePurpose, and revokeConsent to assert the returned response
status explicitly after getResponseOfHttpDelete/getResponseOfHttpPost. Use the
existing response handling in these methods to check for 204 in the delete paths
and 200 in revokeConsent, and fail fast if the status differs so test
setup/teardown cannot continue with stale state.
- Around line 100-108: The createPurpose method uses a brittle String.replace on
the create-purpose.json template to inject the ID, which depends on exact
formatting. Update createPurpose to parse the resource with ObjectMapper, modify
the JSON field directly through an ObjectNode, and then serialize the result
before posting. Keep the existing getResponseOfHttpPost flow, but remove the
hardcoded "\"id\": \"1\"" substitution so the code is resilient to whitespace or
formatting changes in the resource.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/webhooks/consent/ConsentEventTestCase.java`:
- Around line 130-151: The teardown in atEnd() stops on the first cleanup
failure, so later resources like closeHttpClient() and
webhookEventTestManager.teardown() may be skipped. Update
ConsentEventTestCase.atEnd() to wrap each cleanup call (deleteUser,
deletePurpose, deleteElement, closeHttpClient, teardown) in its own guarded
try/catch or equivalent so a failure in one step does not block the rest, and
make sure the cleanup order still uses scim2RestClient,
consentManagementRestClient, and webhookEventTestManager safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5cd718e-61be-46bd-8c46-f7f2834c359e
📒 Files selected for processing (5)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/ConsentManagementRestClient.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/webhooks/consent/ConsentEventTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/webhooks/consent/eventpayloadbuilder/ConsentEventTestExpectedEventPayloadBuilder.javamodules/integration/tests-integration/tests-backend/src/test/resources/testng.xmlpom.xml
| public String createElement() throws Exception { | ||
|
|
||
| String body = readResource("create-element.json"); | ||
| try (CloseableHttpResponse response = getResponseOfHttpPost( | ||
| adminApiBaseUrl + ELEMENTS_PATH, body, getAdminHeaders())) { | ||
| return new ObjectMapper().readTree(readResponse(response)).get("id").asText(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
No HTTP status validation before parsing response body.
createElement, createPurpose, and createConsent all call .get("id").asText() directly on the parsed JSON without checking the response status code first. If the API call fails (e.g., 400/401/500), the response body likely won't contain an id field, and JsonNode.get("id") returns null, causing a NullPointerException on .asText() rather than a clear failure message pointing to the actual HTTP error.
🐛 Proposed fix (illustrated for createElement)
public String createElement() throws Exception {
String body = readResource("create-element.json");
try (CloseableHttpResponse response = getResponseOfHttpPost(
adminApiBaseUrl + ELEMENTS_PATH, body, getAdminHeaders())) {
- return new ObjectMapper().readTree(readResponse(response)).get("id").asText();
+ String responseBody = readResponse(response);
+ int statusCode = response.getStatusLine().getStatusCode();
+ if (statusCode != 201) {
+ throw new IOException("Failed to create element. Status: " + statusCode + ", Body: " + responseBody);
+ }
+ return new ObjectMapper().readTree(responseBody).get("id").asText();
}
}Since this pattern repeats in createPurpose and createConsent, applying the same guard there would improve failure diagnostics.
Also applies to: 100-108, 132-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/ConsentManagementRestClient.java`
around lines 73 - 80, The createElement, createPurpose, and createConsent
methods in ConsentManagementRestClient parse the JSON body and call
get("id").asText() without first validating the HTTP response status. Add a
status check immediately after receiving the CloseableHttpResponse in each
method, and only parse the body when the response indicates success; otherwise
fail with a clear message that includes the HTTP status/body. Use the existing
getResponseOfHttpPost, readResponse, and ObjectMapper flow to keep the fix
consistent across all three methods.
|
PR builder completed |
1 similar comment
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
2 participants
No description provided.