[RHCLOUD-44561] Add system endpoints on Endpoint Api V2#4056
Open
g-duval wants to merge 2 commits intoRedHatInsights:masterfrom
Open
[RHCLOUD-44561] Add system endpoints on Endpoint Api V2#4056g-duval wants to merge 2 commits intoRedHatInsights:masterfrom
g-duval wants to merge 2 commits intoRedHatInsights:masterfrom
Conversation
Contributor
Reviewer's GuideAdds support in the v2 endpoint API to include both regular and system endpoints, exposing a readOnly flag, while keeping v1 behavior unchanged; repository queries and tests are updated to handle optional inclusion of system integrations and their counts. Sequence diagram for V2 getEndpoints including system integrationssequenceDiagram
actor User
participant EndpointResourceV2
participant EndpointResourceCommon
participant EndpointRepository
participant Database
User ->> EndpointResourceV2: GET /api/notifications/v2.0/endpoints
EndpointResourceV2 ->> EndpointResourceCommon: internalGetEndpoints(sec, query, targetType, activeOnly, name, true, true)
EndpointResourceCommon ->> EndpointRepository: getEndpointsPerCompositeType(orgId, name, compositeType, activeOnly, query, true)
EndpointRepository ->> Database: queryBuilderEndpointsPerType(orgId, name, type, activeOnly)
Database -->> EndpointRepository: regular endpoints list
EndpointRepository ->> Database: queryBuilderEndpointsPerType(null, name, type, activeOnly)
Database -->> EndpointRepository: system endpoints list
EndpointRepository -->> EndpointResourceCommon: combined endpoints list
EndpointResourceCommon ->> EndpointRepository: getEndpointsCountPerCompositeType(orgId, name, compositeType, activeOnly, true)
EndpointRepository ->> Database: count endpoints where orgId = orgId
Database -->> EndpointRepository: regular count
EndpointRepository ->> Database: count endpoints where orgId is null
Database -->> EndpointRepository: system count
EndpointRepository -->> EndpointResourceCommon: total count
loop build EndpointDTO list
EndpointResourceCommon ->> EndpointResourceCommon: fromEndpoint(endpoint)
alt system endpoint
EndpointResourceCommon ->> EndpointResourceCommon: endpointDTO.setReadOnly(true)
else regular endpoint
EndpointResourceCommon ->> EndpointResourceCommon: endpointDTO.setReadOnly(false)
end
end
EndpointResourceCommon -->> EndpointResourceV2: EndpointPage
EndpointResourceV2 -->> User: EndpointPage with readOnly flags
Sequence diagram for V2 getEndpoint including system integrationssequenceDiagram
actor User
participant EndpointResourceV2
participant EndpointResourceCommon
participant EndpointRepository
participant Database
User ->> EndpointResourceV2: GET /api/notifications/v2.0/endpoints/{id}
EndpointResourceV2 ->> EndpointResourceCommon: internalGetEndpoint(sec, id, true, true)
EndpointResourceCommon ->> EndpointRepository: getEndpointWithLinkedEventTypes(orgId, id, true)
EndpointRepository ->> Database: SELECT e LEFT JOIN FETCH eventTypes WHERE e.id = id AND (e.orgId = orgId OR e.orgId is null)
Database -->> EndpointRepository: Endpoint or empty
alt endpoint not found
EndpointRepository -->> EndpointResourceCommon: Optional empty
EndpointResourceCommon -->> EndpointResourceV2: throws NotFoundException
EndpointResourceV2 -->> User: 404 Not Found
else endpoint found
EndpointRepository -->> EndpointResourceCommon: Optional with Endpoint
EndpointResourceCommon ->> EndpointResourceCommon: fromEndpoint(endpoint)
EndpointResourceCommon ->> EndpointResourceCommon: includeLinkedEventTypes(endpoint.eventTypes, endpointDTO)
alt system endpoint
EndpointResourceCommon ->> EndpointResourceCommon: endpointDTO.setReadOnly(true)
else regular endpoint
EndpointResourceCommon ->> EndpointResourceCommon: endpointDTO.setReadOnly(false)
end
EndpointResourceCommon -->> EndpointResourceV2: EndpointDTO
EndpointResourceV2 -->> User: EndpointDTO with readOnly
end
Class diagram for updated endpoint repository and resourcesclassDiagram
class EndpointRepository {
+Endpoint createEndpoint(Endpoint endpoint)
+List~Endpoint~ getEndpointsPerCompositeType(String orgId, String name, Set~CompositeEndpointType~ type, Boolean activeOnly, Query limiter, boolean includeSystemIntegrations)
+Long getEndpointsCountPerCompositeType(String orgId, String name, Set~CompositeEndpointType~ type, Boolean activeOnly, boolean includeSystemIntegrations)
+Optional~Endpoint~ getSystemSubscriptionEndpoint(String orgId, SystemSubscriptionProperties properties, EndpointType endpointType)
+Endpoint createSystemSubscriptionEndpoint(String accountId, String orgId, SystemSubscriptionProperties properties, EndpointType endpointType)
+Optional~Endpoint~ getEndpointWithLinkedEventTypes(String orgId, UUID id, boolean includeSystemIntegrationFlag)
}
class EndpointResourceCommon {
-EndpointRepository endpointRepository
+EndpointDTO internalGetEndpoint(SecurityContext securityContext, UUID id, boolean includeLinkedEventTypes, boolean includeSystemIntegrationFlag)
+EndpointPage internalGetEndpoints(SecurityContext sec, Query query, List~String~ targetType, Boolean activeOnly, String name, boolean includeLinkedEventTypes, boolean includeSystemIntegrations)
}
class EndpointResourceV2 {
+EndpointDTO getEndpoint(SecurityContext sec, UUID id)
+EndpointPage getEndpoints(SecurityContext sec, Query query, List~String~ targetType, Boolean activeOnly, String name)
}
class EndpointResource {
+EndpointDTO getEndpoint(SecurityContext sec, UUID id)
+EndpointPage getEndpoints(SecurityContext sec, Query query, List~String~ targetType, Boolean activeOnly, String name)
}
class EndpointDTO {
+UUID id
+String name
+String description
+Set~UUID~ eventTypes
+Boolean readOnly
+UUID getId()
+void setId(UUID id)
+String getName()
+void setName(String name)
+String getDescription()
+void setDescription(String description)
+Set~UUID~ getEventTypes()
+void setEventTypes(Set~UUID~ eventTypes)
+Boolean getReadOnly()
+void setReadOnly(Boolean readOnly)
}
EndpointResourceCommon --> EndpointRepository : uses
EndpointResourceV2 --> EndpointResourceCommon : delegates
EndpointResource --> EndpointResourceCommon : delegates
EndpointResourceV2 --> EndpointDTO : returns
EndpointResource --> EndpointDTO : returns
EndpointRepository --> Endpoint : manages
EndpointDTO --> Endpoint : represents
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
getEndpointsPerCompositeType/getEndpointsCountPerCompositeTypechanges build the final list and count by combining two separate queries (org-specific and system) after applying limit/sort, which will break pagination and global ordering (e.g. limit is effectively applied per query, and the combined list may not respect the requested sort); consider unifying this into a single query or applying limit/sort after combining results to keep paging semantics consistent. - When
includeSystemIntegrationsis true ingetEndpointsPerCompositeType, system endpoints are appended after org-specific endpoints, which means they will always appear after all org-specific items regardless of sorting; if system and org endpoints should share the same global ordering, you may want to push the system inclusion into the query itself instead of merging in Java.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `getEndpointsPerCompositeType`/`getEndpointsCountPerCompositeType` changes build the final list and count by combining two separate queries (org-specific and system) after applying limit/sort, which will break pagination and global ordering (e.g. limit is effectively applied per query, and the combined list may not respect the requested sort); consider unifying this into a single query or applying limit/sort after combining results to keep paging semantics consistent.
- When `includeSystemIntegrations` is true in `getEndpointsPerCompositeType`, system endpoints are appended after org-specific endpoints, which means they will always appear after all org-specific items regardless of sorting; if system and org endpoints should share the same global ordering, you may want to push the system inclusion into the query itself instead of merging in Java.
## Individual Comments
### Comment 1
<location> `backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EndpointRepository.java:92-95` </location>
<code_context>
}
- public List<Endpoint> getEndpointsPerCompositeType(String orgId, @Nullable String name, Set<CompositeEndpointType> type, Boolean activeOnly, Query limiter) {
+ public List<Endpoint> getEndpointsPerCompositeType(String orgId, @Nullable String name, Set<CompositeEndpointType> type, Boolean activeOnly, Query limiter, boolean includeSystemIntegrations) {
Query.Limit limit = limiter == null ? null : limiter.getLimit();
Optional<Sort> sort = limiter == null ? Optional.empty() : Sort.getSort(limiter, null, Endpoint.SORT_FIELDS);
</code_context>
<issue_to_address>
**issue (bug_risk):** Combining two separately sorted & limited queries can break global ordering and pagination semantics.
When `includeSystemIntegrations` is `true`, we run two queries with the same `limit` and `sort` and then concatenate them. This means:
* The final list is not globally sorted; each subset is sorted, but their concatenation may violate the requested sort order.
* A `limit` of N can yield up to 2N items, and the resulting page is not equivalent to applying `limit` and `sort` over a single unified dataset.
To preserve the API’s apparent contract of a single, sorted, paginated collection, consider either:
* A single query that includes both org-specific and system endpoints in the `WHERE` clause, or
* Merging both result sets in memory with the requested sort and then applying `limit` once.
If this divergence from global pagination is intentional, it would help to document clearly that this method returns two separately paginated lists concatenated together so callers don’t assume a single consistent page.
</issue_to_address>
### Comment 2
<location> `backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/endpoint/EndpointResourceTest.java:3856-3865` </location>
<code_context>
+ @Test
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for system endpoints combined with filters/pagination and multiple system integrations
The new `testCreateThenReadRegularAndSystemEndpoint` covers V1 vs V2 visibility and `readOnly` for a single regular + system endpoint. To better lock in the new behavior, consider adding tests that:
- List endpoints via V2 with filters (`targetType`, `active`, `name`) to confirm system endpoints are (or aren’t) included as expected.
- Use multiple system endpoints of different types to confirm all appear in V2 with `readOnly = true` while V1 still ignores them.
- (Optional) Handle an org with only system endpoints (no org-bound ones) to confirm V2 still returns them and V1 returns an empty list.
This would strengthen coverage around `includeSystemIntegrations` and `readOnly` handling and help prevent regressions.
Suggested implementation:
```java
@Test
void testCreateThenReadRegularAndSystemEndpoint() {
// Create event type and endpoint
final Endpoint endpoint = resourceHelpers.createEndpoint(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, EndpointType.WEBHOOK);
final Endpoint systemEndpoint = resourceHelpers.createEndpoint(null, null, EndpointType.EMAIL_SUBSCRIPTION);
final String identityHeaderValue = TestHelpers.encodeRHIdentityInfo(TestConstants.DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, "username");
final Header identityHeader = TestHelpers.createRHIdentityHeader(identityHeaderValue);
MockServerConfig.addMockRbacAccess(identityHeaderValue, FULL_ACCESS);
}
@Test
void testListEndpointsV2WithFiltersIncludesSystemEndpoints() {
// Arrange: regular + system endpoints
final Endpoint regularWebhook = resourceHelpers.createEndpoint(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, EndpointType.WEBHOOK);
final Endpoint systemEmail = resourceHelpers.createEndpoint(null, null, EndpointType.EMAIL_SUBSCRIPTION);
final String identityHeaderValue = TestHelpers.encodeRHIdentityInfo(TestConstants.DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, "username");
final Header identityHeader = TestHelpers.createRHIdentityHeader(identityHeaderValue);
MockServerConfig.addMockRbacAccess(identityHeaderValue, FULL_ACCESS);
// Act: list endpoints via V2 with filters that should match both regular + system endpoints
final io.restassured.path.json.JsonPath json = io.restassured.RestAssured
.given()
.header(identityHeader)
.queryParam("includeSystemIntegrations", true)
.queryParam("targetType", EndpointType.WEBHOOK.toString())
.queryParam("active", true)
.when()
.get("/api/notifications/v2.0/endpoints") // adjust path if different in this test class
.then()
.statusCode(200)
.extract()
.jsonPath();
@SuppressWarnings("unchecked")
final java.util.List<java.util.Map<String, Object>> data = json.getList("data");
// Assert: regular endpoint is present and writable
final boolean regularFound = data.stream().anyMatch(e ->
DEFAULT_ACCOUNT_ID.equals(e.get("accountId")) &&
DEFAULT_ORG_ID.equals(e.get("orgId")) &&
regularWebhook.getId().toString().equals(String.valueOf(e.get("id"))) &&
Boolean.FALSE.equals(e.get("readOnly"))
);
// Assert: system endpoint is present and read-only when includeSystemIntegrations = true
final boolean systemFoundReadOnly = data.stream().anyMatch(e ->
e.get("accountId") == null &&
e.get("orgId") == null &&
systemEmail.getId().toString().equals(String.valueOf(e.get("id"))) &&
Boolean.TRUE.equals(e.get("readOnly"))
);
org.junit.jupiter.api.Assertions.assertTrue(regularFound, "Regular endpoint should be included in V2 list with filters");
org.junit.jupiter.api.Assertions.assertTrue(systemFoundReadOnly, "System endpoint should be included in V2 list with filters and marked readOnly");
}
@Test
void testMultipleSystemEndpointsVisibleInV2AndIgnoredInV1() {
// Arrange: regular endpoint for the org + multiple system endpoints
final Endpoint regularWebhook = resourceHelpers.createEndpoint(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, EndpointType.WEBHOOK);
final Endpoint systemEmail = resourceHelpers.createEndpoint(null, null, EndpointType.EMAIL_SUBSCRIPTION);
final Endpoint systemWebhook = resourceHelpers.createEndpoint(null, null, EndpointType.WEBHOOK);
final String identityHeaderValue = TestHelpers.encodeRHIdentityInfo(TestConstants.DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, "username");
final Header identityHeader = TestHelpers.createRHIdentityHeader(identityHeaderValue);
MockServerConfig.addMockRbacAccess(identityHeaderValue, FULL_ACCESS);
// Act: list via V1 (should ignore system integrations)
final io.restassured.path.json.JsonPath v1Json = io.restassured.RestAssured
.given()
.header(identityHeader)
.when()
.get("/endpoints") // adjust path if different in this test class
.then()
.statusCode(200)
.extract()
.jsonPath();
@SuppressWarnings("unchecked")
final java.util.List<java.util.Map<String, Object>> v1Data = v1Json.getList("data");
// Assert: V1 only contains the org-bound endpoint(s)
final boolean v1ContainsRegular = v1Data.stream().anyMatch(e ->
DEFAULT_ACCOUNT_ID.equals(e.get("accountId")) &&
DEFAULT_ORG_ID.equals(e.get("orgId")) &&
regularWebhook.getId().toString().equals(String.valueOf(e.get("id")))
);
final boolean v1ContainsSystem = v1Data.stream().anyMatch(e ->
e.get("accountId") == null && e.get("orgId") == null
);
org.junit.jupiter.api.Assertions.assertTrue(v1ContainsRegular, "V1 list should contain the org-bound endpoint");
org.junit.jupiter.api.Assertions.assertFalse(v1ContainsSystem, "V1 list should not contain system endpoints");
// Act: list via V2 with includeSystemIntegrations = true
final io.restassured.path.json.JsonPath v2Json = io.restassured.RestAssured
.given()
.header(identityHeader)
.queryParam("includeSystemIntegrations", true)
.when()
.get("/api/notifications/v2.0/endpoints") // adjust path if different in this test class
.then()
.statusCode(200)
.extract()
.jsonPath();
@SuppressWarnings("unchecked")
final java.util.List<java.util.Map<String, Object>> v2Data = v2Json.getList("data");
// Assert: V2 contains regular + both system endpoints, with all system endpoints readOnly
final long regularCount = v2Data.stream().filter(e ->
DEFAULT_ACCOUNT_ID.equals(e.get("accountId")) &&
DEFAULT_ORG_ID.equals(e.get("orgId")) &&
Boolean.FALSE.equals(e.get("readOnly"))
).count();
final long systemCount = v2Data.stream().filter(e ->
e.get("accountId") == null &&
e.get("orgId") == null &&
Boolean.TRUE.equals(e.get("readOnly"))
).count();
org.junit.jupiter.api.Assertions.assertEquals(1, regularCount, "V2 list should contain exactly one regular endpoint for the org");
org.junit.jupiter.api.Assertions.assertTrue(systemCount >= 2, "V2 list should contain both system endpoints and mark them as readOnly");
}
@Test
void testOrgWithOnlySystemEndpointsV2ReturnsThemWhileV1IsEmpty() {
// Arrange: only system endpoints exist for this test
final Endpoint systemEmail = resourceHelpers.createEndpoint(null, null, EndpointType.EMAIL_SUBSCRIPTION);
final Endpoint systemWebhook = resourceHelpers.createEndpoint(null, null, EndpointType.WEBHOOK);
final String identityHeaderValue = TestHelpers.encodeRHIdentityInfo(TestConstants.DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, "username");
final Header identityHeader = TestHelpers.createRHIdentityHeader(identityHeaderValue);
MockServerConfig.addMockRbacAccess(identityHeaderValue, FULL_ACCESS);
// Act: V1 list should be empty for this org/account
final io.restassured.path.json.JsonPath v1Json = io.restassured.RestAssured
.given()
.header(identityHeader)
.when()
.get("/endpoints") // adjust path if different in this test class
.then()
.statusCode(200)
.extract()
.jsonPath();
@SuppressWarnings("unchecked")
final java.util.List<java.util.Map<String, Object>> v1Data = v1Json.getList("data");
org.junit.jupiter.api.Assertions.assertTrue(v1Data == null || v1Data.isEmpty(),
"V1 list should be empty when only system endpoints exist");
// Act: V2 list with includeSystemIntegrations = true should still return system endpoints
final io.restassured.path.json.JsonPath v2Json = io.restassured.RestAssured
.given()
.header(identityHeader)
.queryParam("includeSystemIntegrations", true)
.when()
.get("/api/notifications/v2.0/endpoints") // adjust path if different in this test class
.then()
.statusCode(200)
.extract()
.jsonPath();
@SuppressWarnings("unchecked")
final java.util.List<java.util.Map<String, Object>> v2Data = v2Json.getList("data");
final boolean emailSystemFound = v2Data.stream().anyMatch(e ->
e.get("accountId") == null &&
e.get("orgId") == null &&
systemEmail.getId().toString().equals(String.valueOf(e.get("id"))) &&
Boolean.TRUE.equals(e.get("readOnly"))
);
final boolean webhookSystemFound = v2Data.stream().anyMatch(e ->
e.get("accountId") == null &&
e.get("orgId") == null &&
systemWebhook.getId().toString().equals(String.valueOf(e.get("id"))) &&
Boolean.TRUE.equals(e.get("readOnly"))
);
org.junit.jupiter.api.Assertions.assertTrue(emailSystemFound, "V2 list should include the system email endpoint and mark it as readOnly");
org.junit.jupiter.api.Assertions.assertTrue(webhookSystemFound, "V2 list should include the system webhook endpoint and mark it as readOnly");
}
```
1. Adjust the V1 and V2 endpoint URLs used in the tests (`/endpoints` and `/api/notifications/v2.0/endpoints`) to match the existing conventions in `EndpointResourceTest` (e.g. they might be `/api/notifications/v1.0/endpoints`, `/api/notifications/v2.0/endpoints`, or include a base path from a constant).
2. If the JSON structure returned by the list endpoints does not use `data` as the array field, or if field names differ (`accountId`, `orgId`, `id`, `readOnly`), update the JSONPath expressions and map key lookups accordingly.
3. If there’s already a helper to list endpoints (e.g. `listEndpointsV1`, `listEndpointsV2`, or a common `getEndpoints(...)` method), replace the direct RestAssured calls with those helpers to keep the tests consistent with the rest of the class.
4. Ensure `RestAssured` and `JsonPath` static imports are consistent with the rest of the file (you may prefer static imports like `given()`, `when()`, `then()` and `assertThat` instead of fully qualified names).
5. If the test class uses a different identity/mock RBAC setup pattern (e.g. helper methods to build `identityHeader` and grant permissions), reuse those helpers in these tests instead of duplicating logic.
6. Verify that the test data setup (especially `resourceHelpers.createEndpoint(null, null, ...)`) is the same mechanism used elsewhere in the test suite to create system endpoints; if system endpoints require specific flags or types, adjust the `EndpointType` and creation logic accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EndpointRepository.java
Show resolved
Hide resolved
...test/java/com/redhat/cloud/notifications/routers/handlers/endpoint/EndpointResourceTest.java
Show resolved
Hide resolved
g-duval
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why?
In order to be able to remove the Behavior Group management screen, we have to add system integration into regular integrations/endpoints apis.
Since end users will understand why they are notify even if they didn't configured such integration.
For UI purpose, those system integrations as to be flagged as read only.
Summary by Sourcery
Expose system (org-less) endpoints via the v2 endpoints API while keeping them hidden in v1, and label them as read-only in responses.
New Features:
Enhancements:
Tests: