Use extended cache for caching integration configuration details

[brant-livefront]

📔 Objective

This PR moves the most high-traffic piece of event integrations over to the new extended cache. This part is the processing that happens every time when receive an event. We need to fetch the integrations from the database and determine if an integration should be fire. Previously, we had a home-grown database that would load all integrations into memory on a fixed interval. This PR replaces that with extended cache.

There are a number of benefits to having this in extended cache:

• We don't need to pre-load any data. We can fetch from the database as events first come in (with stampede protection) and the results will exist in the cache
• Due to extended cache's redis support, that newly cached item will be in redis for other nodes that see the same event type / organization / integration type later.
• As a result of the backplane, we can invalidate the cache (flushing it from L2 / redis) as soon as an admin changes any integration or integration configurations and this will be propagated to all nodes (flushing their L1 / memory cache). The next event that would need this will then load the brand new data on first request and refill the L1 / L2 caches.
• Since we no longer need to worry about stale data, we can use a very long (1 day) TTL on these records which allows us to lean even more heavily on cache.
• We are relying on known implementations and don't need to worry about maintaining our own hand-coded in-memory cache.

In addition I've cleaned up the docs and fixed an error that I noticed while testing this (Trying to create an integration of the same type as an existing one would 500 error. It now returns a BadRequestException - and we have a new test to validate that)

Finally, I have updated the method / stored procedure / query for fetching configurations. It now includes configurations where EventType is null, which match all events. So asking for all the configurations that match a specific event type, integration type and organization id will retrieve:

1. Those specific rows that have that event type explicitly (this is the same as before)
2. Any rows that match integration type and organization id, but have a null event type (this is the new addition). Again, nulls are considered as wildcards so they are now included here.

Note: I've moved the changes for Controllers (the write side of this PR) into 2 separate PRs since the changes were large. They will follow this PR shortly.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @brant-livefront's task —— View job

---

Code Review Complete ✅

I've conducted a comprehensive code review of this PR. Overall, this is a well-structured migration to extended cache with significant benefits. The implementation is solid with good test coverage and clear documentation updates.

---

📊 Summary

This PR successfully migrates event integration configuration caching from a custom in-memory solution to the extended cache system (FusionCache). The key improvement is consolidating two separate cache lookups into a single call that retrieves both explicit event type matches and wildcard (null event type) matches in one query.

Key Changes:

• ✅ Removed custom IntegrationConfigurationDetailsCacheService
• ✅ Updated EventIntegrationHandler to use FusionCache with 1-day TTL
• ✅ Combined wildcard and explicit event type queries into single stored procedure
• ✅ Enhanced cache key generation and tagging
• ✅ Comprehensive test coverage added
• ✅ Excellent documentation updates in README.md

---

✅ Strengths

1. Excellent Architecture Decision

The migration to extended cache provides significant benefits:

• Stampede protection built-in
• Multi-tier caching (L1 memory + L2 Redis)
• Cache invalidation via backplane
• No need to pre-load data

2. Query Optimization

The latest commit (395b9ad) improves efficiency by:

• Combining two cache calls into one
• Single stored procedure returns both explicit and wildcard matches
• Cleaner cache key structure

3. Strong Test Coverage

The test suite is comprehensive with 224 added lines covering:

• Cache factory behavior
• Configuration retrieval with tags
• TTL validation
• Cache key uniqueness

4. Documentation Excellence

The README.md updates clearly explain:

• Caching strategy and benefits
• Cache key/tag usage patterns
• TTL rationale (1 day with invalidation)

---

🔍 Issues Found

⚠️ MEDIUM: Missing Null Check in EventIntegrationHandler

File: src/Core/AdminConsole/Services/Implementations/EventIntegrations/EventIntegrationHandler.cs:145-148

The repository method GetManyByEventTypeOrganizationIdIntegrationType could potentially return null from the cache factory, but the code doesn't handle this case:

configurations.AddRange(await cache.GetOrSetAsync<List<OrganizationIntegrationConfigurationDetails>>(
    key: EventIntegrationsCacheConstants.BuildCacheKeyForOrganizationIntegrationConfigurationDetails(...),
    factory: async _ => await configurationRepository.GetManyByEventTypeOrganizationIdIntegrationType(...),
    options: new FusionCacheEntryOptions(...),
    tags: [integrationTag]
));

Issue: If the repository returns null, AddRange will throw ArgumentNullException.

Recommendation: Add null-coalescing operator:

configurations.AddRange(await cache.GetOrSetAsync<List<OrganizationIntegrationConfigurationDetails>>(
    key: EventIntegrationsCacheConstants.BuildCacheKeyForOrganizationIntegrationConfigurationDetails(...),
    factory: async _ => await configurationRepository.GetManyByEventTypeOrganizationIdIntegrationType(...) ?? [],
    options: new FusionCacheEntryOptions(...),
    tags: [integrationTag]
) ?? []);

📝 MINOR: Inconsistent XML Documentation

File: src/Core/AdminConsole/Repositories/IOrganizationIntegrationConfigurationRepository.cs:12-15

The XML comment has formatting inconsistencies with extra <br/> tags that aren't used elsewhere in the codebase:

/// <summary>
/// Retrieve the list of available configuration details for a specific event for the organization and
/// integration type.<br/>
/// <br/>
/// <b>Note:</b> This returns all configurations that match the event type explicitly <b>and</b>

Recommendation: Follow standard C# XML comment style:

/// <summary>
/// Retrieve the list of available configuration details for a specific event for the organization and
/// integration type.
/// <para>
/// Note: This returns all configurations that match the event type explicitly and
/// all the configurations that have a null event type - null event type is considered a
/// wildcard that matches all events.
/// </para>
/// </summary>

📝 MINOR: Magic Number in Comment

File: src/Core/AdminConsole/Services/Implementations/EventIntegrations/EventIntegrationHandler.cs:88

// Note: All of these cache calls use the default options, including TTL of 30 minutes

This comment is misleading - it references a 30-minute TTL, but the code doesn't actually specify TTL for these calls, meaning they use FusionCache's default. Consider clarifying or removing this comment.

---

✨ Suggestions for Improvement

1. Cache Constant for BuildContext TTL

File: src/Core/Utilities/EventIntegrationsCacheConstants.cs

The comment in EventIntegrationHandler mentions "30 minutes" but this isn't defined as a constant. Consider adding:

/// <summary>
/// Default cache duration for template properties (User, Group, Organization lookups).
/// Uses FusionCache default TTL.
/// </summary>
public static readonly TimeSpan? DurationForTemplateProperties = null; // Uses default

This makes the intent explicit and provides a single place to change it if needed.

2. Consider Adding Integration Tests

While unit tests are comprehensive, integration tests that verify the actual cache behavior with Redis/Memory would strengthen confidence in the caching strategy, especially around:

• Cache invalidation via tags
• Multi-tier cache behavior
• Stampede protection under load

3. Repository Return Type Guarantee

Files: Both Dapper and EF implementations

Consider updating both repository implementations to guarantee non-null returns:

public async Task<List<OrganizationIntegrationConfigurationDetails>>
    GetManyByEventTypeOrganizationIdIntegrationType(...)
{
    // ... existing code ...
    return results?.ToList() ?? new List<OrganizationIntegrationConfigurationDetails>();
}

This makes the contract clearer and prevents null-related issues upstream.

---

🔒 Security Assessment

✅ No security concerns identified

• No sensitive data exposure in cache keys
• Proper organization ID scoping
• No SQL injection risks (parameterized queries)
• Cache invalidation properly scoped by organization/integration

---

📈 Performance Considerations

Positive Impact:

• ✅ 1-day TTL with cache invalidation reduces DB load significantly
• ✅ Single query instead of two separate calls
• ✅ Redis-backed L2 cache enables cross-node sharing
• ✅ Stampede protection prevents duplicate DB queries

Monitoring Recommendations:

• Monitor cache hit rates for OrganizationIntegrationConfigurationDetails
• Track cache invalidation frequency when integrations are modified
• Verify backplane propagation latency in production

---

✅ Verification of Previous Review Comments

• ✅ "Wildcard" terminology removed - Addressed in commit 5eb8c0e
• ✅ Combined query approach - Implemented in commit 395b9ad
• ✅ Migration date updated - File dated 2025-12-05

---

🎯 Alignment with Project Guidelines

CLAUDE.md Compliance:

• ✅ No code regions used
• ✅ Unit tests added for new caching functionality
• ✅ No sensitive data in logs - cache keys use GUIDs
• ✅ Nullable reference types respected
• ✅ Performance considered - significant improvement expected

Code Quality:

• ✅ Clean separation of concerns
• ✅ Dependency injection properly used
• ✅ Async/await patterns correct
• ✅ Exception handling appropriate

---

📋 Final Recommendation

Status: APPROVED with minor suggestions ✅

This PR is ready to merge after addressing the null-check issue (medium priority). The architectural improvements are solid, test coverage is excellent, and the documentation updates are thorough.

Required Before Merge:

1. ⚠️ Add null checks/coalescing in EventIntegrationHandler:145

Optional Improvements:

2. 📝 Clean up XML documentation formatting
3. 📝 Clarify or remove the "30 minutes" comment
4. ✨ Consider adding cache TTL constant

---

📊 Test Coverage Analysis

Patch Coverage: 68.75% (15 lines missing coverage)

Missing Coverage Areas:

1. OrganizationIntegrationConfigurationDetailsReadManyByEventTypeOrganizationIdIntegrationTypeQuery.cs - 8 lines (EF query construction)
2. ServiceCollectionExtensions.cs - 4 lines (DI registration)
3. OrganizationIntegrationConfigurationRepository.cs (EF) - 3 lines (repository method)

Analysis: The missing coverage is primarily in infrastructure/setup code (DI, EF queries) which is acceptable. Core business logic in EventIntegrationHandler is well-covered.

---

Great work on this refactoring! The migration to extended cache is well-executed and will provide significant operational benefits. 🎉

---

[github-actions]

Checkmarx One – Scan Summary & Details – 38b740d3-c8dc-4952-8009-bf1c70f61269

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1519	details Method at line 1519 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: dMGF5qNfAN72zlvQcA1MgbhHv%2Fc%3D Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1395	details Method at line 1395 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: iOCFr11iI9znjDnv46yLfiS4aDY%3D Attack Vector

Fixed Issues (3)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 489
	CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 97
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 300

[codecov]

Codecov Report

❌ Patch coverage is 68.75000% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.27%. Comparing base (18a8829) to head (5eb8c0e).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...nyByEventTypeOrganizationIdIntegrationTypeQuery.cs	0.00%	8 Missing ⚠️
...SharedWeb/Utilities/ServiceCollectionExtensions.cs	0.00%	4 Missing ⚠️
.../OrganizationIntegrationConfigurationRepository.cs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6650      +/-   ##
==========================================
- Coverage   57.27%   57.27%   -0.01%     
==========================================
  Files        1917     1916       -1     
  Lines       85518    85471      -47     
  Branches     7673     7672       -1     
==========================================
- Hits        48983    48954      -29     
+ Misses      34691    34672      -19     
- Partials     1844     1845       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mkincaid-bw]

Is this stored procedure name appropriate? With 3 predicates in the where clause, will there be more than 1 result per OrganiztionId and IntegrationType? Also, in ReadManyWildcard, we don't use Wildcard anywhere else in the DB. It's fine if it's applicable, just haven't seen it before.

[withinfocus]

I was good with the name when I first read it to make the openness clear, but yeah we can probably just get rid of "wildcard". Yes, there will be many.

[brant-livefront]

Yeah, I wasn't 100% sold on "Wildcard" either. I landed on that to call out that we're fetching ones where EventType is explicitly null, which we treat as a wildcard event - they match all events. I'd be up for dropping it if we think it's misleading.

[withinfocus]

Yeah, just get rid of it.

[brant-livefront]

Latest commit has a refactor to remove references to "wildcard". 👍

[withinfocus]

I didn't formally request changes since we were discussing this on Slack, but my primary request besides my current inline comments was that we need a service or CQRS to be in place here for the cache invocations -- they should not be out at the edge in controllers.

[withinfocus]

🎨 Cache constant for all these.

[withinfocus]

I was good with the name when I first read it to make the openness clear, but yeah we can probably just get rid of "wildcard". Yes, there will be many.

[brant-livefront]

Claude's issues 1 and 5 are both addressed in the follow on PR for this: #6675

The other Claude comments I'm fine with as-is, but let me know if anyone has concerns.

[brant-livefront]

@withinfocus et al - I've pulled all of the Controller / API related changes out of this PR. I'll put those up (with the CQRS changes) in two separate PRs. But I wanted to slim this one down to just the DB changes, the EventIntegrationHandler changes and some fixes / supporting changes for those.

[brant-livefront]

I spoke with @withinfocus and we've altered the strategy a bit, and the latest commit executes on this direction. Instead of making 2 calls to the cache (one to fetch matching records that explicitly match the event type and a second call to fetch the records that match with a null event type) we are simply combining this into one call. The net-net is I've removed all of my new stored procedure / method / query setup (which fetched only null event types). Instead, this PR now updates the existing stored procedure to include the "wildcard" matches that have null event type. So that becomes the one call we are making to the cache and then to the repository.

Before:

1. Call to retrieve specific event type matches
2. Call to retrieve null event type matches

After:

1. Call to retrieve configs that match via direct event type match and and configs that match via null event type wildcard match

This is obviously effect the cache clearing, but that is being handled in the PRs that are dependent on this. I'm making those updates next.

[withinfocus]

Latest commit looks good, but bump the date on the stored proc migration -- won't be an issue in practice but the current date is pretty drifted.