Cache calls to SecurityTypeByISIN #18

Merged

natercio merged 2 commits from cache-figi-responses into main 2025-11-25 13:52:05 +00:00

Conversation 2 Commits 2 Files Changed 2

+82 -1

natercio commented 2025-11-25 13:06:14 +00:00

Owner

Copy Link

Copy Source

This avoids unecessary repeated calls to OpenFIGI api

This avoids unecessary repeated calls to OpenFIGI api

natercio added 1 commit 2025-11-25 13:06:14 +00:00

cache calls to SecurityTypeByISIN ... 57ee768006

This avoids unecessary repeated calls to OpenFIGI api

natercio commented 2025-11-25 13:07:07 +00:00

Author

Owner

Copy Link

Copy Source

@claude review this PR

@claude review this PR

gitea-actions bot commented 2025-11-25 13:07:17 +00:00

Copy Link

Copy Source

Claude finished @natercio's task —— View job

---

PR Review: Cache calls to SecurityTypeByISIN

Todo List:

• Read the changed files to understand the caching implementation
• Check for repo-specific guidelines (CLAUDE.md)
• Analyze caching mechanism for correctness and potential issues
• Review test coverage and quality
• Provide comprehensive review feedback

---

Review Summary

This PR implements a simple in-memory cache for OpenFIGI API calls to avoid repeated requests for the same ISIN. The implementation is generally solid but has a few areas for improvement.

✅ Positive Aspects

1. Good thread safety: Proper use of sync.RWMutex with separate read/write locks
2. Clean cache-first approach: Check cache before making API calls
3. Comprehensive test coverage: Added dedicated test for cache functionality
4. Respects rate limiting: Cache doesn't interfere with existing rate limiter
5. Memory efficient: Simple map-based cache without unnecessary complexity

⚠️ Issues and Recommendations

Critical: Memory Leak Risk

Location: internal/open_figi.go:30,95-96

The cache grows indefinitely without any eviction policy. In a long-running application, this could lead to memory exhaustion.

Recommendations:

• Add a maximum cache size with LRU eviction
• Implement TTL-based expiration for cached entries
• Consider using a library like golang-lru or go-cache

Minor: Lock Optimization

Location: internal/open_figi.go:35-40

The current pattern of unlock/relock could be optimized:

// Current approach
of.mu.RLock()
if secType, ok := of.securityTypeCache[isin]; ok {
    of.mu.RUnlock()
    return secType, nil
}
of.mu.RUnlock()

Suggestion: This is actually fine as-is. The unlock before return in the cache hit case is correct.

Minor: Error Handling

Location: internal/open_figi.go:95-96

Consider what happens if the API returns a successful response but with invalid/empty security type. The cache will store empty values.

Suggestion: Add validation before caching:

if secType == "" {
    return "", fmt.Errorf("empty security type returned for ISIN %s", isin)
}

Minor: Test Enhancement

Location: internal/open_figi_test.go:121-155

The cache test only verifies that API isn't called twice but doesn't test concurrent access patterns.

Suggestion: Add a test for concurrent cache access to ensure thread safety.

📋 Code Quality

• Code style: Consistent with existing codebase
• Documentation: The comment on line 90-91 about ISIN uniqueness is helpful
• Error handling: Consistent with existing patterns
• Type safety: Proper use of Go's type system

🧪 Test Quality

The new cache test TestOpenFIGI_SecurityTypeByISIN_Cache effectively verifies:

• Cache hit avoids API calls
• Returned values are consistent
• Integration with existing functionality

Verdict: APPROVE with suggestions

The implementation successfully achieves the goal of caching OpenFIGI calls. While there are some minor improvements that could be made (particularly around memory management), the current implementation is safe and functional for typical usage patterns.

Priority: Address the memory leak risk if this service processes large volumes of unique ISINs.

---

**Claude finished @natercio's task** —— [View job](http://gitea:3000/applications/any2anexoj/actions/runs/87) --- ### PR Review: Cache calls to SecurityTypeByISIN **Todo List:** - [x] Read the changed files to understand the caching implementation - [x] Check for repo-specific guidelines (CLAUDE.md) - [x] Analyze caching mechanism for correctness and potential issues - [x] Review test coverage and quality - [x] Provide comprehensive review feedback --- ### Review Summary This PR implements a simple in-memory cache for OpenFIGI API calls to avoid repeated requests for the same ISIN. The implementation is generally solid but has a few areas for improvement. ### ✅ Positive Aspects 1. **Good thread safety**: Proper use of `sync.RWMutex` with separate read/write locks 2. **Clean cache-first approach**: Check cache before making API calls 3. **Comprehensive test coverage**: Added dedicated test for cache functionality 4. **Respects rate limiting**: Cache doesn't interfere with existing rate limiter 5. **Memory efficient**: Simple map-based cache without unnecessary complexity ### ⚠️ Issues and Recommendations #### **Critical: Memory Leak Risk** **Location**: `internal/open_figi.go:30,95-96` The cache grows indefinitely without any eviction policy. In a long-running application, this could lead to memory exhaustion. **Recommendations**: - Add a maximum cache size with LRU eviction - Implement TTL-based expiration for cached entries - Consider using a library like `golang-lru` or `go-cache` #### **Minor: Lock Optimization** **Location**: `internal/open_figi.go:35-40` The current pattern of unlock/relock could be optimized: ```go // Current approach of.mu.RLock() if secType, ok := of.securityTypeCache[isin]; ok { of.mu.RUnlock() return secType, nil } of.mu.RUnlock() ``` **Suggestion**: This is actually fine as-is. The unlock before return in the cache hit case is correct. #### **Minor: Error Handling** **Location**: `internal/open_figi.go:95-96` Consider what happens if the API returns a successful response but with invalid/empty security type. The cache will store empty values. **Suggestion**: Add validation before caching: ```go if secType == "" { return "", fmt.Errorf("empty security type returned for ISIN %s", isin) } ``` #### **Minor: Test Enhancement** **Location**: `internal/open_figi_test.go:121-155` The cache test only verifies that API isn't called twice but doesn't test concurrent access patterns. **Suggestion**: Add a test for concurrent cache access to ensure thread safety. ### 📋 Code Quality - **Code style**: Consistent with existing codebase - **Documentation**: The comment on line 90-91 about ISIN uniqueness is helpful - **Error handling**: Consistent with existing patterns - **Type safety**: Proper use of Go's type system ### 🧪 Test Quality The new cache test `TestOpenFIGI_SecurityTypeByISIN_Cache` effectively verifies: - Cache hit avoids API calls - Returned values are consistent - Integration with existing functionality ### Verdict: **APPROVE with suggestions** The implementation successfully achieves the goal of caching OpenFIGI calls. While there are some minor improvements that could be made (particularly around memory management), the current implementation is safe and functional for typical usage patterns. **Priority**: Address the memory leak risk if this service processes large volumes of unique ISINs. ---

natercio added 1 commit 2025-11-25 13:50:21 +00:00

error on empty securityType and more efficient locks 64bbf8d129

natercio merged commit 4a2884a0df into main 2025-11-25 13:52:05 +00:00

natercio deleted branch cache-figi-responses 2025-11-25 13:52:05 +00:00

natercio referenced this issue from a commit 2025-11-25 13:52:06 +00:00

Merge pull request 'Cache calls to SecurityTypeByISIN' (#18) from cache-figi-responses into main

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Assignees

Clear assignees

natercio (Natercio Moniz)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: applications/any2anexoj#18