MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition #434

bvsharp · 2024-09-24T20:03:04Z

Purpose

MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition

bvsharp · 2024-09-25T18:11:05Z

src/main/java/org/folio/fqm/service/CrossTenantQueryService.java

-    if (!forceCrossTenantQuery && !Boolean.TRUE.equals(entityType.getCrossTenantQueriesEnabled())) {
+    if (!forceCrossTenantQuery
+      && !Boolean.TRUE.equals(entityType.getCrossTenantQueriesEnabled())
+      && !COMPOSITE_INSTANCES_ID.equals(entityType.getId())) {


I hate how complicated this condition is getting, but we do need all these checks. The new check for the composite instances ID is due to the fact that cross-tenant queries will be disabled for non-central tenants, but we DO still need to run a (pseudo) cross tenant query for composite instances in member tenants in order to get the shared instances from the central tenant.

This is going to run into issues with the tenant ID dropdown menu for instances, since the entity type that gets passed in is simple_instances, which doesn't have cross-tenant queries enabled :(

If we end up needing to check both composite and simple instances in 2 places in this method, I think I'd make a private static final Set<String> to hold the IDs, then do a contains(entityType.getId()) instead of checking both IDs separately

This situation actually gets handled by the forceCrossTenantQuery parameter! When we get the tenant ID values, we pass forceCrossTenantQuery=true to this method. Since it's true, we skip this block. From there:

If ECS is not enabled, we return only the current tenant.

If ECS is enabled but we're not in the central tenant AND the entity type is composite/simple instances, we return the current tenant plus the central tenant (in order to get the shared instances).

If ECS is enabled and we're in the central tenant, then we query everything we have permission for.

So I think everything related to the tenant ID dropdown is handled. The only thing that the current code doesn't handle is running a (non-tenant ID related) query on the simple_instances entity from a member tenant. In that situation, we'd only be querying the member tenant, so we wouldn't see the shared records from the central tenant (although this is counter-balanced by the fact that simple_instances doesn't have an additionalEcsConditions defined either, meaning that we wouldn't be filtering out shadow instances. So in this situation we'd see ALMOST the same thing as for composite_instances, only we'd be seeing the shadow instances instead of the shared ones).

And I don't think we'd want to include simple_instances in the first check (at least the way we have the definitions set up right now), due to the fact the simple_instances doesn't filter out the shadow instances. So if we added the simple_instance check to the first block, we'd end up with duplicate instance records in the query results (one shadow version and one shared version for each shared instance).

bvsharp · 2024-09-25T18:52:47Z

src/main/java/org/folio/fqm/service/EntityTypeService.java

+    boolean crossTenantEnabled = Boolean.TRUE.equals(entityType.getCrossTenantQueriesEnabled())
+      && crossTenantQueryService.isCentralTenant();


Kind of a weird way to do it, but this essentially overwrites the crossTenantQueriesEnabled property, so that non-central tenants have it set to false, even if it's true in the original definition. This will allow us to avoid forcing lists for cross-tenant entity types to be private in member tenants, and instead only apply that effect to lists in the central tenant. Still not crazy about this approach but it seems to be the simplest option for now.

mweaver-ebsco · 2024-09-25T19:00:55Z

src/main/java/org/folio/fqm/service/CrossTenantQueryService.java

+  public boolean isCentralTenant() {
+    return executionContext.getTenantId().equals(getCentralTenantId());
+  }


I'd follow the pattern that the other method here use, where there's overloaded with a private version that takes the Map<String, String> ecsTenantInfo parameter. Then, I'd update getTenantsToQuery() to use the private version, so that all of the "is this the central tenant?" logic runs through that one method

bvsharp · 2024-09-26T21:29:02Z

src/main/java/org/folio/fqm/service/UserTenantService.java

+package org.folio.fqm.service;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.log4j.Log4j2;
+import org.folio.fqm.client.SimpleHttpClient;
+import org.springframework.cache.annotation.Cacheable;
+import org.springframework.stereotype.Service;
+
+import java.util.Map;
+
+/**
+ * Service wrapper for caching responses from user-tenants API.
+ */
+@Service
+@RequiredArgsConstructor
+@Log4j2
+public class UserTenantService {
+
+  private final SimpleHttpClient userTenantsClient;
+
+  @Cacheable(value="userTenantCache", key="#tenantId")
+  public String getUserTenantsResponse(String tenantId) {
+    log.info("Retrieving user-tenants information for tenant {}", tenantId);
+    return userTenantsClient.get("user-tenants", Map.of("limit", String.valueOf(1)));
+  }
+}


Added this class to allow for convenient caching for the user-tenants responses. We use the /user-tenants endpoint in both the EntityTypeFlatteningService and the CrossTenantQueryService, so I figured adding a service wrapper made the most sense to allow us to:

Cache the user-tenants responses in a convenient way regardless of what class the request is made from, and without code duplication.

Avoid caching responses on any of the other SimpleHttpClient requests.

bvsharp · 2024-09-26T21:30:32Z

src/main/resources/application.yml

@@ -75,6 +75,7 @@ coffee-boots:
  cache:
    spec:
      queryCache: maximumSize=500,expireAfterWrite=1m
+      userTenantCache: maximumSize=100,expireAfterWrite=5h


Picked a fairly arbitrary expire time, and am open to any other values for this cache. Also open to making it a configurable value, though I didn't do make it configurable for the time being, since I'm not sure there's any value in doing so.

…ing definition

sonarcloud · 2024-09-26T21:42:42Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mweaver-ebsco · 2024-09-26T22:39:21Z

src/main/java/org/folio/fqm/service/UserTenantService.java

Good call on breaking this out into a separate service!

bvsharp force-pushed the MODFQMMGR-456-take-3 branch 4 times, most recently from bc0a1b3 to 7204a0f Compare September 25, 2024 17:12

bvsharp commented Sep 25, 2024

View reviewed changes

bvsharp marked this pull request as ready for review September 25, 2024 18:29

bvsharp commented Sep 25, 2024

View reviewed changes

bvsharp requested review from mweaver-ebsco, ncovercash and kjain110 September 25, 2024 18:55

ncovercash approved these changes Sep 25, 2024

View reviewed changes

mweaver-ebsco reviewed Sep 25, 2024

View reviewed changes

bvsharp force-pushed the MODFQMMGR-456-take-3 branch 3 times, most recently from 9dd919c to a5bdafb Compare September 26, 2024 21:25

bvsharp commented Sep 26, 2024

View reviewed changes

MODFQMMGR-456: Check whether entity type is cross-tenant when retriev…

e748fd7

…ing definition

bvsharp force-pushed the MODFQMMGR-456-take-3 branch from a5bdafb to e748fd7 Compare September 26, 2024 21:37

mweaver-ebsco reviewed Sep 26, 2024

View reviewed changes

mweaver-ebsco approved these changes Sep 26, 2024

View reviewed changes

kjain110 approved these changes Sep 27, 2024

View reviewed changes

bvsharp merged commit 5280cfa into master Sep 27, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition #434

MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition #434

bvsharp commented Sep 24, 2024

bvsharp Sep 25, 2024

mweaver-ebsco Sep 25, 2024

mweaver-ebsco Sep 25, 2024

bvsharp Sep 25, 2024 •

edited

Loading

bvsharp Sep 25, 2024

bvsharp Sep 25, 2024

mweaver-ebsco Sep 25, 2024

bvsharp Sep 26, 2024 •

edited

Loading

bvsharp Sep 26, 2024 •

edited

Loading

sonarcloud bot commented Sep 26, 2024

mweaver-ebsco Sep 26, 2024

		boolean crossTenantEnabled = Boolean.TRUE.equals(entityType.getCrossTenantQueriesEnabled())
		&& crossTenantQueryService.isCentralTenant();

MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition #434

MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition #434

Conversation

bvsharp commented Sep 24, 2024

Purpose

bvsharp Sep 25, 2024

Choose a reason for hiding this comment

mweaver-ebsco Sep 25, 2024

Choose a reason for hiding this comment

mweaver-ebsco Sep 25, 2024

Choose a reason for hiding this comment

bvsharp Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

bvsharp Sep 25, 2024

Choose a reason for hiding this comment

bvsharp Sep 25, 2024

Choose a reason for hiding this comment

mweaver-ebsco Sep 25, 2024

Choose a reason for hiding this comment

bvsharp Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

bvsharp Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Sep 26, 2024

Quality Gate passed

mweaver-ebsco Sep 26, 2024

Choose a reason for hiding this comment

bvsharp Sep 25, 2024 •

edited

Loading

bvsharp Sep 26, 2024 •

edited

Loading

bvsharp Sep 26, 2024 •

edited

Loading