Caching autoDiscovery result#6
Conversation
PR Summary
|
📝 WalkthroughWalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Cache
participant HTTPClient as HTTP Client
participant ConfigParser as Config Parser
Caller->>Cache: Check for cached discovery<br/>(key: oidc:discovery:md5(...))
alt Cache Hit
Cache-->>Caller: Return cached config
else Cache Miss
Cache->>HTTPClient: Fetch discovery document
HTTPClient-->>Cache: Response (status, body)
alt Response OK
Cache->>ConfigParser: Parse configuration
ConfigParser-->>Cache: Parsed config
Cache->>Cache: Store in cache (TTL: 3600s)
else Response Not OK
Cache->>Cache: Return null
end
Cache-->>Caller: Return config or null
end
Caller->>ConfigParser: Process configuration<br/>(if config is truthy)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements caching for the OpenID Connect discovery process within the AutoDiscovery trait to improve performance. The feedback highlights two main areas for improvement: ensuring cache key consistency by sorting query parameters before hashing, and replacing the direct use of the env() helper with the config() helper to prevent issues when Laravel's configuration is cached.
| if ($provider_url) { | ||
| $response = $this->client() | ||
| ->get("$provider_url/$this->DISCOVERY_PATH", $query_params); | ||
| $cacheKey = 'oidc:discovery:' . md5(json_encode([$provider_url, $query_params])); |
There was a problem hiding this comment.
The cache key generation is sensitive to the order of keys in the $query_params array. If the same parameters are passed in a different order, it will result in different MD5 hashes and unnecessary cache misses. To ensure consistent cache hits, consider sorting the query parameters (e.g., using ksort()) before encoding them into the cache key.
| ->get("$provider_url/$this->DISCOVERY_PATH", $query_params); | ||
| $cacheKey = 'oidc:discovery:' . md5(json_encode([$provider_url, $query_params])); | ||
| /** @noinspection LaravelFunctionsInspection */ | ||
| $cacheTtl = env('OIDC_DISCOVERY_CACHE_TTL', 3600); |
There was a problem hiding this comment.
Avoid using the env() helper directly in application logic. In Laravel, env() should ideally only be used within configuration files. When the application configuration is cached (via php artisan config:cache), all env() calls outside of configuration files return null. This causes the code to ignore the actual environment variable and fall back to the default value of 3600. It is recommended to define a configuration key (e.g., in config/openid.php) and use the config() helper instead.
There was a problem hiding this comment.
Since this is not a Laravel-specific library, but a generic PHP one, it should be better to get the ttl from a config parameter instead of an env
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Traits/AutoDiscovery.php (1)
42-42: Canonicalize query params before hashing cache key.Line 42 currently hashes raw
$query_params; equivalent associative arrays with different key order will generate different keys and reduce cache hit rate.♻️ Suggested change
- $cacheKey = 'oidc:discovery:' . md5(json_encode([$provider_url, $query_params])); + $normalizedQueryParams = $query_params; + if (is_array($normalizedQueryParams)) { + ksort($normalizedQueryParams); + } + $cacheKey = 'oidc:discovery:' . md5(json_encode([$provider_url, $normalizedQueryParams]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Traits/AutoDiscovery.php` at line 42, The cache key generation is using raw $query_params which can vary by key order; update the AutoDiscovery trait so before computing $cacheKey you canonicalize $query_params (e.g., recursively sort associative arrays by keys) then encode that canonicalized structure (with $provider_url) for md5; locate the line building $cacheKey and replace the input to json_encode with a deterministic/canonicalized version of $query_params so equivalent param sets always produce the same $cacheKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Traits/AutoDiscovery.php`:
- Line 42: The cache key generation is using raw $query_params which can vary by
key order; update the AutoDiscovery trait so before computing $cacheKey you
canonicalize $query_params (e.g., recursively sort associative arrays by keys)
then encode that canonicalized structure (with $provider_url) for md5; locate
the line building $cacheKey and replace the input to json_encode with a
deterministic/canonicalized version of $query_params so equivalent param sets
always produce the same $cacheKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 456c6d35-530b-40a5-8488-4656fbea42fb
📒 Files selected for processing (1)
src/Traits/AutoDiscovery.php
|
Hi @DrNixx, thank you for your PR! The feature is great and useful, but I agree with Gemini comments that should be addressed (also check my comment in the second one). After this, I'll be happy to merge it! |
2 participants
List of common tasks a pull request require complete
Summary by CodeRabbit