backend/inmem-cleanup-rel-imprv#1333
Conversation
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. WalkthroughThis PR enhances in-memory cache management with cross-pod coordination through Redis notifications. Type contracts are extended to track cleanup notifications, the core cleanup thread separates TTL expiry from size-based eviction, handler APIs are updated to send Redis notifications on cache refresh, and HTTP client resilience is added to retry on connection failures. ChangesCross-pod cache cleanup coordination via Redis
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/mobility-core/src/Kernel/Storage/InMem.hs (1)
165-183:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis force-cleanup flag becomes sticky on long-lived pods.
The check is keyed off
updatedCacheInfo.createdAt, which is cache-wide and usually much older than the refresh notification. Once the condition becomes true,expire key 600renews the Redis marker on every cleanup pass, so matching entries keep getting removed every minute even after they were recached. Compare against per-entry creation timestamps or consume/delete the marker after one successful sweep instead of extending its TTL in the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/mobility-core/src/Kernel/Storage/InMem.hs` around lines 165 - 183, The current cleanup renews the Redis marker by calling Hedis.expire key 600 inside the cache-wide createdAt check, making the force-cleanup sticky; fix by NOT renewing the marker in the cleanup loop and either (A) compare each entry's own creation timestamp (e.g. use InMemKeyInfo.createdAt or similar per-entry time on v in the fold) against forceCleanupTimestamp (from forceCleanupValue) to decide removal, or (B) perform the cache-wide sweep once and then remove/consume the marker (Hedis.del key) instead of calling Hedis.expire; also remove the Hedis.expire key 600 call from this branch so matching entries are not repeatedly re-removed. Ensure references: updatedCacheInfo, forceCleanupTimestamp, forceCleanupKeyPrefix, InMemKeyInfo, InMemCacheInfo, and the Hedis.expire key call are updated accordingly.lib/mobility-core/src/Kernel/Types/CacheFlow.hs (1)
94-98:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStore a full timestamp here, not
TimeOfDay.This value is written by the refresh paths and later compared against cache state in
lib/mobility-core/src/Kernel/Storage/InMem.hs. Dropping the date means a refresh near UTC midnight can be skipped on other pods because23:59and00:05are no longer orderable once the day is lost. UseUTCTimeor another absolute timestamp in the Redis payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/mobility-core/src/Kernel/Types/CacheFlow.hs` around lines 94 - 98, The struct ForceCleanupExpiryValue currently uses TimeOfDay for forceCleanupTimestamp which loses date information; change forceCleanupTimestamp to UTCTime (Data.Time.Clock) and update its FromJSON/ToJSON usage (or derive instances) so the Redis payload stores an absolute timestamp; then update all places that construct or compare ForceCleanupExpiryValue (e.g., refresh paths that create it and the comparison logic in InMem.hs that orders the timestamp) to produce/get UTCTime (use getCurrentTime when creating) and to compare UTCTime values instead of TimeOfDay, and add the necessary import for UTCTime/getCurrentTime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mobility-core/src/Kernel/Storage/InMem.hs`:
- Around line 108-115: Both refresh paths currently overwrite the single Redis
key "inmem:force:cleanup:timeofday" (setExp) causing missed invalidations;
change the design to append events instead of replacing them: update the writers
(the code that builds ForceCleanupExpiryValue and calls
runInMultiCloudRedisWrite setExp "inmem:force:cleanup:timeofday") to push the
payload into a Redis append structure (e.g., LPUSH to
"inmem:force:cleanup:events" or XADD to a stream) including
forceCleanupTimestamp and forceCleanupKeyPrefix/keyInfix, and update
inMemCleanupThread to consume all queued events (BRPOP/LPOP loop or
XREAD/XREADGROUP) and process each payload rather than reading one shared key;
ensure the event list/stream has retention (trim/expire) so old entries are
removed.
In `@lib/mobility-core/src/Kernel/Storage/InMem/Management/Handler.hs`:
- Around line 78-92: The refresh path currently does a non-atomic readIORef then
writeIORef on inMemHashMap (inside refreshInMem handler using req.keyInfix),
which can lose concurrent inserts; change the logic to use atomicModifyIORef so
the snapshot and update happen atomically: perform the filter/clear and
recompute cacheSize inside an atomicModifyIORef on (inMemHashMap inMemEnv),
returning the new InMemCacheInfo and the deleted/remaining counts; reference the
existing InMemCacheInfo type, the inMemHashMap IORef, req.keyInfix, and keep
consistency with how refreshInMem already uses atomicModifyIORef and how
withInMemCache performs concurrent inserts.
In `@lib/mobility-core/src/Kernel/Storage/InMem/Management/Types.hs`:
- Around line 70-85: The OpenAPI schema for PodCleanupAck is out-of-sync with
the Aeson JSON rename: podCleanupAckJsonOptions maps errorMessage → "error" for
ToJSON/FromJSON but PodCleanupAck still derives ToSchema (so the schema will
expose "errorMessage"); update this by providing an explicit ToSchema
PodCleanupAck that uses the same Aeson options (e.g., use
genericDeclareUnNamedSchema/fromAesonOptions with podCleanupAckJsonOptions) so
the generated schema matches the runtime JSON keys (affecting PodCleanupAck and
SidecarRefreshResponse.pods), or alternatively remove the rename in
podCleanupAckJsonOptions so schema and JSON already align.
---
Outside diff comments:
In `@lib/mobility-core/src/Kernel/Storage/InMem.hs`:
- Around line 165-183: The current cleanup renews the Redis marker by calling
Hedis.expire key 600 inside the cache-wide createdAt check, making the
force-cleanup sticky; fix by NOT renewing the marker in the cleanup loop and
either (A) compare each entry's own creation timestamp (e.g. use
InMemKeyInfo.createdAt or similar per-entry time on v in the fold) against
forceCleanupTimestamp (from forceCleanupValue) to decide removal, or (B) perform
the cache-wide sweep once and then remove/consume the marker (Hedis.del key)
instead of calling Hedis.expire; also remove the Hedis.expire key 600 call from
this branch so matching entries are not repeatedly re-removed. Ensure
references: updatedCacheInfo, forceCleanupTimestamp, forceCleanupKeyPrefix,
InMemKeyInfo, InMemCacheInfo, and the Hedis.expire key call are updated
accordingly.
In `@lib/mobility-core/src/Kernel/Types/CacheFlow.hs`:
- Around line 94-98: The struct ForceCleanupExpiryValue currently uses TimeOfDay
for forceCleanupTimestamp which loses date information; change
forceCleanupTimestamp to UTCTime (Data.Time.Clock) and update its
FromJSON/ToJSON usage (or derive instances) so the Redis payload stores an
absolute timestamp; then update all places that construct or compare
ForceCleanupExpiryValue (e.g., refresh paths that create it and the comparison
logic in InMem.hs that orders the timestamp) to produce/get UTCTime (use
getCurrentTime when creating) and to compare UTCTime values instead of
TimeOfDay, and add the necessary import for UTCTime/getCurrentTime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83f092ab-0b09-4533-b7d7-8605cd3ea50a
📒 Files selected for processing (5)
lib/mobility-core/src/Kernel/Storage/InMem.hslib/mobility-core/src/Kernel/Storage/InMem/Management/Handler.hslib/mobility-core/src/Kernel/Storage/InMem/Management/SidecarClient.hslib/mobility-core/src/Kernel/Storage/InMem/Management/Types.hslib/mobility-core/src/Kernel/Types/CacheFlow.hs
| -- Write to Redis forceCleanup key so other pods also clean up | ||
| let cleanupTimeOfDay = timeToTimeOfDay (utctDayTime now) | ||
| val = | ||
| ForceCleanupExpiryValue | ||
| { forceCleanupTimestamp = cleanupTimeOfDay, | ||
| forceCleanupKeyPrefix = Just keyInfix | ||
| } | ||
| runInMultiCloudRedisWrite $ setExp "inmem:force:cleanup:timeofday" val 600 |
There was a problem hiding this comment.
A single Redis marker will drop refresh events.
Both refresh paths write the same inmem:force:cleanup:timeofday key, while inMemCleanupThread polls once per minute and reads only one payload. If two refreshes happen before the next poll, the second write overwrites the first and some pods will miss one invalidation entirely. This needs a queue/stream/per-event key strategy instead of replacing one shared value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/Storage/InMem.hs` around lines 108 - 115, Both
refresh paths currently overwrite the single Redis key
"inmem:force:cleanup:timeofday" (setExp) causing missed invalidations; change
the design to append events instead of replacing them: update the writers (the
code that builds ForceCleanupExpiryValue and calls runInMultiCloudRedisWrite
setExp "inmem:force:cleanup:timeofday") to push the payload into a Redis append
structure (e.g., LPUSH to "inmem:force:cleanup:events" or XADD to a stream)
including forceCleanupTimestamp and forceCleanupKeyPrefix/keyInfix, and update
inMemCleanupThread to consume all queued events (BRPOP/LPOP loop or
XREAD/XREADGROUP) and process each payload rather than reading one shared key;
ensure the event list/stream has retention (trim/expire) so old entries are
removed.
| oldInfo <- liftIO $ readIORef (inMemHashMap inMemEnv) | ||
| let oldCount = HM.size (cache oldInfo) | ||
| case req.keyInfix of | ||
| (deletedCount, remainingCount) <- case req.keyInfix of | ||
| Nothing -> do | ||
| liftIO $ | ||
| writeIORef (inMemHashMap inMemEnv) $ | ||
| InMemCacheInfo {cache = HM.empty, cacheSize = 0, createdAt = now} | ||
| pure InMemRefreshResponse {deletedKeys = oldCount, remainingKeys = 0} | ||
| pure (oldCount, 0) | ||
| Just infix_ -> do | ||
| let toKeep = HM.filterWithKey (\k _ -> not (infix_ `T.isInfixOf` k)) (cache oldInfo) | ||
| newSize = foldl' (\acc info -> acc + info.cacheDataSize) 0 (HM.elems toKeep) | ||
| deletedCount = HM.size (cache oldInfo) - HM.size toKeep | ||
| newSize = foldl' (\acc keyInfo -> acc + keyInfo.cacheDataSize) 0 (HM.elems toKeep) | ||
| liftIO $ | ||
| writeIORef (inMemHashMap inMemEnv) $ | ||
| InMemCacheInfo {cache = toKeep, cacheSize = newSize, createdAt = oldInfo.createdAt} | ||
| pure | ||
| InMemRefreshResponse | ||
| { deletedKeys = deletedCount, | ||
| remainingKeys = HM.size toKeep | ||
| pure (HM.size (cache oldInfo) - HM.size toKeep, HM.size toKeep) |
There was a problem hiding this comment.
Make the local refresh update atomic.
This snapshots the cache with readIORef and then overwrites it with writeIORef. A concurrent withInMemCache insert between those calls is lost, even when that key does not match req.keyInfix. refreshInMem already uses atomicModifyIORef; this path needs the same pattern.
Suggested fix
- oldInfo <- liftIO $ readIORef (inMemHashMap inMemEnv)
- let oldCount = HM.size (cache oldInfo)
- (deletedCount, remainingCount) <- case req.keyInfix of
- Nothing -> do
- liftIO $
- writeIORef (inMemHashMap inMemEnv) $
- InMemCacheInfo {cache = HM.empty, cacheSize = 0, createdAt = now}
- pure (oldCount, 0)
- Just infix_ -> do
- let toKeep = HM.filterWithKey (\k _ -> not (infix_ `T.isInfixOf` k)) (cache oldInfo)
- newSize = foldl' (\acc keyInfo -> acc + keyInfo.cacheDataSize) 0 (HM.elems toKeep)
- liftIO $
- writeIORef (inMemHashMap inMemEnv) $
- InMemCacheInfo {cache = toKeep, cacheSize = newSize, createdAt = oldInfo.createdAt}
- pure (HM.size (cache oldInfo) - HM.size toKeep, HM.size toKeep)
+ (deletedCount, remainingCount) <- liftIO $
+ atomicModifyIORef (inMemHashMap inMemEnv) $ \oldInfo ->
+ case req.keyInfix of
+ Nothing ->
+ let oldCount = HM.size (cache oldInfo)
+ newInfo = InMemCacheInfo {cache = HM.empty, cacheSize = 0, createdAt = now}
+ in (newInfo, (oldCount, 0))
+ Just infix_ ->
+ let toKeep = HM.filterWithKey (\k _ -> not (infix_ `T.isInfixOf` k)) (cache oldInfo)
+ newSize = foldl' (\acc keyInfo -> acc + keyInfo.cacheDataSize) 0 (HM.elems toKeep)
+ deletedCount = HM.size (cache oldInfo) - HM.size toKeep
+ newInfo = InMemCacheInfo {cache = toKeep, cacheSize = newSize, createdAt = oldInfo.createdAt}
+ in (newInfo, (deletedCount, HM.size toKeep))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oldInfo <- liftIO $ readIORef (inMemHashMap inMemEnv) | |
| let oldCount = HM.size (cache oldInfo) | |
| case req.keyInfix of | |
| (deletedCount, remainingCount) <- case req.keyInfix of | |
| Nothing -> do | |
| liftIO $ | |
| writeIORef (inMemHashMap inMemEnv) $ | |
| InMemCacheInfo {cache = HM.empty, cacheSize = 0, createdAt = now} | |
| pure InMemRefreshResponse {deletedKeys = oldCount, remainingKeys = 0} | |
| pure (oldCount, 0) | |
| Just infix_ -> do | |
| let toKeep = HM.filterWithKey (\k _ -> not (infix_ `T.isInfixOf` k)) (cache oldInfo) | |
| newSize = foldl' (\acc info -> acc + info.cacheDataSize) 0 (HM.elems toKeep) | |
| deletedCount = HM.size (cache oldInfo) - HM.size toKeep | |
| newSize = foldl' (\acc keyInfo -> acc + keyInfo.cacheDataSize) 0 (HM.elems toKeep) | |
| liftIO $ | |
| writeIORef (inMemHashMap inMemEnv) $ | |
| InMemCacheInfo {cache = toKeep, cacheSize = newSize, createdAt = oldInfo.createdAt} | |
| pure | |
| InMemRefreshResponse | |
| { deletedKeys = deletedCount, | |
| remainingKeys = HM.size toKeep | |
| pure (HM.size (cache oldInfo) - HM.size toKeep, HM.size toKeep) | |
| (deletedCount, remainingCount) <- liftIO $ | |
| atomicModifyIORef (inMemHashMap inMemEnv) $ \oldInfo -> | |
| case req.keyInfix of | |
| Nothing -> | |
| let oldCount = HM.size (cache oldInfo) | |
| newInfo = InMemCacheInfo {cache = HM.empty, cacheSize = 0, createdAt = now} | |
| in (newInfo, (oldCount, 0)) | |
| Just infix_ -> | |
| let toKeep = HM.filterWithKey (\k _ -> not (infix_ `T.isInfixOf` k)) (cache oldInfo) | |
| newSize = foldl' (\acc keyInfo -> acc + keyInfo.cacheDataSize) 0 (HM.elems toKeep) | |
| deletedCount = HM.size (cache oldInfo) - HM.size toKeep | |
| newInfo = InMemCacheInfo {cache = toKeep, cacheSize = newSize, createdAt = oldInfo.createdAt} | |
| in (newInfo, (deletedCount, HM.size toKeep)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/Storage/InMem/Management/Handler.hs` around
lines 78 - 92, The refresh path currently does a non-atomic readIORef then
writeIORef on inMemHashMap (inside refreshInMem handler using req.keyInfix),
which can lose concurrent inserts; change the logic to use atomicModifyIORef so
the snapshot and update happen atomically: perform the filter/clear and
recompute cacheSize inside an atomicModifyIORef on (inMemHashMap inMemEnv),
returning the new InMemCacheInfo and the deleted/remaining counts; reference the
existing InMemCacheInfo type, the inMemHashMap IORef, req.keyInfix, and keep
consistency with how refreshInMem already uses atomicModifyIORef and how
withInMemCache performs concurrent inserts.
| data PodCleanupAck = PodCleanupAck | ||
| { podName :: Text, | ||
| success :: Bool, | ||
| errorMessage :: Maybe Text | ||
| } | ||
| deriving (Generic, Show, ToSchema) | ||
|
|
||
| podCleanupAckJsonOptions :: Ae.Options | ||
| podCleanupAckJsonOptions = Ae.defaultOptions {Ae.fieldLabelModifier = \f -> if f == "errorMessage" then "error" else f} | ||
|
|
||
| instance ToJSON PodCleanupAck where | ||
| toJSON = Ae.genericToJSON podCleanupAckJsonOptions | ||
|
|
||
| instance FromJSON PodCleanupAck where | ||
| parseJSON = Ae.genericParseJSON podCleanupAckJsonOptions | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="lib/mobility-core/src/Kernel/Storage/InMem/Management/Types.hs"
# 1) Show pragmas + imports around the file top
echo "===== Top of $FILE (pragmas/imports) ====="
sed -n '1,140p' "$FILE" | nl -ba
# 2) Show the full definition area around lines 60-110
echo
echo "===== Around PodCleanupAck (lines 50-120) ====="
sed -n '50,130p' "$FILE" | nl -ba
# 3) Find SidecarRefreshResponse definition nearby or where PodCleanupAck is used
echo
echo "===== Find SidecarRefreshResponse references in file ====="
rg -n "SidecarRefreshResponse|PodCleanupAck" "$FILE"
# 4) Show SidecarRefreshResponse definition (small region around any match)
for m in $(rg -n "SidecarRefreshResponse|PodCleanupAck" "$FILE" | cut -d: -f1 | sort -n | uniq); do
start=$((m-25)); end=$((m+25));
echo
echo "===== Context around line $m ====="
sed -n "${start},${end}p" "$FILE" | nl -ba
done | head -n 3000Repository: nammayatri/shared-kernel
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for schema/openapi generation / occurrences
echo "===== Repo search: PodCleanupAck ====="
rg -n "PodCleanupAck" -S .
echo
echo "===== Repo search: errorMessage within Mobility core ====="
rg -n "errorMessage" lib/mobility-core -S
echo
echo "===== Repo search: Ae.fieldLabelModifier usage ====="
rg -n "fieldLabelModifier" -S .
echo
echo "===== Repo search: genericToSchema / ToSchema options ====="
rg -n "generic.*ToSchema|ToSchema.*Options|json.*schema.*Options|genericToSchema" -S lib/mobility-core
echo
echo "===== Repo search: openapi/Swagger snapshots ====="
rg -n "(swagger|openapi|OpenAPI|Swagger|spec\\.json|swagger\\.json|openapi\\.json)" -S lib/mobility-coreRepository: nammayatri/shared-kernel
Length of output: 19967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="lib/mobility-core/src/Kernel/Storage/InMem/Management/Types.hs"
echo "===== Top of $FILE (pragmas/imports) ====="
sed -n '1,160p' "$FILE" | nl -ba
echo
echo "===== Around PodCleanupAck (lines 60-110) ====="
sed -n '60,115p' "$FILE" | nl -ba
echo
echo "===== Find SidecarRefreshResponse definition lines ====="
rg -n "data SidecarRefreshResponse|SidecarRefreshResponse" "$FILE"Repository: nammayatri/shared-kernel
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="lib/mobility-core/src/Kernel/Storage/InMem/Management/Types.hs"
echo "===== Top of $FILE (pragmas/imports) ====="
sed -n '1,200p' "$FILE" | nl -ba
echo
echo "===== Around PodCleanupAck (lines 60-110) ====="
sed -n '55,120p' "$FILE" | nl -ba
echo
echo "===== Lines containing SidecarRefreshResponse / PodCleanupAck in file ====="
rg -n "SidecarRefreshResponse|PodCleanupAck" "$FILE"
echo
echo "===== Context around SidecarRefreshResponse occurrences ====="
while IFS=: read -r line rest; do
start=$((line-25)); end=$((line+35))
echo
echo "=== ${FILE}:${line} ==="
sed -n "${start},${end}p" "$FILE" | nl -ba
done < <(rg -n "SidecarRefreshResponse" "$FILE")
echo
echo "===== Repo search: PodCleanupAck ====="
rg -n "PodCleanupAck" -S .
echo
echo "===== Repo search: errorMessage ====="
rg -n "errorMessage" -S .
echo
echo "===== Repo search: fieldLabelModifier ====="
rg -n "fieldLabelModifier" -S .Repository: nammayatri/shared-kernel
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect local helpers for ToSchema naming conventions
echo "===== Search for ToSchema jsonOptions usage in mobility-core ====="
rg -n "genericToJSON|genericParseJSON|genericToSchema|genericDeclare|defaultOptions|Ae\\.defaultOptions" -S lib/mobility-core
echo
echo "===== Search for 'fieldLabelModifier =\\' in mobility-core ====="
rg -n "fieldLabelModifier\s*=\s*\\\\" -S lib/mobility-core
echo
echo "===== Search for ToSchema derived with custom json options elsewhere ====="
rg -n "defaultOptions.*fieldLabelModifier" -S lib/mobility-coreRepository: nammayatri/shared-kernel
Length of output: 50380
🌐 Web query:
servant-openapi3 ToSchema deriving Generic record field names respects aeson fieldLabelModifier
💡 Result:
Yes, the Generic implementation for ToSchema in the servant-openapi3 (via the openapi3 package) library supports customizing record field names using a mechanism similar to Aeson, but it requires explicit configuration of SchemaOptions [1][2]. While Aeson's generic derivation uses its own Options type, openapi3 provides a corresponding SchemaOptions type [1][2]. You can manually specify a fieldLabelModifier in SchemaOptions to transform record field names during schema generation [1][2]. To use custom field label modification: 1. Define your own SchemaOptions instance with the desired fieldLabelModifier [1][2]. 2. Use genericDeclareNamedSchema or genericDeclareSchema instead of the default ToSchema instance, passing your custom SchemaOptions [3][4]. Alternatively, the library provides a helper function, fromAesonOptions, which can convert Aeson Options into SchemaOptions [1][2]. This allows you to reuse the same logic you used for your Aeson ToJSON/FromJSON instances [1][2]. Note that while fromAesonOptions copies the fieldLabelModifier, it only affects the fields that map directly to the SchemaOptions type; other Aeson-specific options may be ignored by the schema generator [1][2]. If you are using the default deriving clause (deriving ToSchema), it uses the defaultSchemaOptions (which has fieldLabelModifier = id), meaning it will not automatically pick up your Aeson configuration unless you manually define the ToSchema instance to use your customized SchemaOptions [1][5][4].
Citations:
- 1: https://hackage.haskell.org/package/openapi3-3.2.4/docs/Data-OpenApi-SchemaOptions.html
- 2: https://www.stackage.org/haddock/lts-24.32/openapi3-3.2.4/Data-OpenApi-SchemaOptions.html
- 3: https://hackage.haskell.org/package/openapi3-3.2.4/docs/Data-OpenApi-Internal-Schema.html
- 4: https://hackage.haskell.org/package/openapi3-3.2.4/docs/src/Data.OpenApi.Internal.Schema.html
- 5: https://docs.servant.dev/en/latest/cookbook/openapi3/OpenAPI.html
🌐 Web query:
servant-openapi3 genericToSchema aeson Options fieldLabelModifier
💡 Result:
To customize field names when generating schemas with servant-openapi3 (and the underlying openapi3 library), you use the SchemaOptions type [1][2]. If you already have Aeson Options defined for your generic JSON instances, you can convert them directly to SchemaOptions using the fromAesonOptions function [1][3]. This function automatically maps the fieldLabelModifier, constructorTagModifier, allNullaryToStringTag, and unwrapUnaryRecords fields from your Aeson Options to the corresponding SchemaOptions [1][3]. For manual configuration, define a custom SchemaOptions record where fieldLabelModifier is a function of type String -> String [1][4]. Example implementation: import Data.OpenApi import GHC.Generics data User = User { userName:: String, userEmail:: String } deriving (Generic) -- Define custom options mySchemaOptions:: SchemaOptions mySchemaOptions = defaultSchemaOptions { fieldLabelModifier = drop 4 -- Example: removes "user" prefix } instance ToSchema User where declareNamedSchema proxy = genericDeclareNamedSchema mySchemaOptions proxy When syncing with existing Aeson generic instances (e.g., genericToJSON), ensure your SchemaOptions match the logic in your Aeson Options to guarantee the generated OpenAPI specification accurately reflects your API's JSON output [5][6]. Note that while fromAesonOptions copies essential fields, some Aeson options like omitNothingFields have no effect on SchemaOptions and must be handled separately if needed [1][3].
Citations:
- 1: https://hackage.haskell.org/package/openapi3-3.2.4/docs/Data-OpenApi-SchemaOptions.html
- 2: https://hackage.haskell.org/package/openapi3-3.2.4/docs/Data-OpenApi-Schema.html
- 3: https://hackage.haskell.org/package/openapi3-3.2.3/docs/Data-OpenApi-Schema.html
- 4: https://www.stackage.org/haddock/lts-24.32/openapi3-3.2.4/Data-OpenApi-ParamSchema.html
- 5: https://hackage.haskell.org/package/servant-openapi3-2.0.1.6/docs/Servant-OpenApi.html
- 6: [haskell-servant][haskell-yesod] Use table-based conversion for field name conversion OpenAPITools/openapi-generator#16232
🌐 Web query:
deriveToSchema fieldLabelModifier aeson Options
💡 Result:
In the Haskell aeson library, fieldLabelModifier is a field within the Options data type used to customize how Haskell record field names are mapped to JSON keys [1][2]. The fieldLabelModifier function has the signature (String -> String) and is applied to each record field name during instance derivation [3][2]. When using Template Haskell for derivation via functions like deriveJSON, you modify the defaultOptions record to include your custom modifier [1][4]: $(deriveJSON defaultOptions { fieldLabelModifier = drop 4 } ''MyDataType) If you are using GHC.Generics-based derivation (via genericToJSON, genericParseJSON, etc.), you similarly apply the modifier to the Options object [2][5]: customOptions:: Options customOptions = defaultOptions { fieldLabelModifier = drop 1 } instance ToJSON MyDataType where toJSON = genericToJSON customOptions Common patterns for fieldLabelModifier include: - Removing prefixes: Using functions like drop or custom string manipulation to strip leading underscores or type prefixes from field names [1][2][4]. - Converting case: Using helper functions like camelTo2 from Data.Aeson.Types to convert camelCase Haskell fields to snake_case JSON keys [2]. - Mapping specific names: Using a lambda or named function to rename specific fields (e.g., to avoid reserved keywords or match specific API requirements) [3]. Always ensure that you use the same Options value for both ToJSON and FromJSON instances to maintain consistency between encoding and decoding [2].
Citations:
- 1: https://hackage.haskell.org/package/aeson/docs/Data-Aeson-TH.html
- 2: https://jsonic.io/guides/json-haskell
- 3: https://stackoverflow.com/questions/48474587/how-to-deal-with-haskells-reserved-keywords-in-record-fields
- 4: https://mmhaskell.com/blog/2017/6/5/flexible-data-with-aeson
- 5: https://hackage.haskell.org/package/aeson-2.2.1.0/docs/Data-Aeson.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use sed/cat since `nl` isn't available in the sandbox.
FILE="lib/mobility-core/src/Kernel/Storage/InMem/Management/Types.hs"
echo "===== PodCleanupAck + SidecarRefreshResponse (around lines 60-110) ====="
sed -n '60,110p' "$FILE" | cat -n
echo
echo "===== Repo search: fromAesonOptions usage ====="
rg -n "fromAesonOptions" -S lib/mobility-core || true
echo
echo "===== Inspect Kernel/Utils/Schema.hs for schema options helpers ====="
SCHEMA_FILE="lib/mobility-core/src/Kernel/Utils/Schema.hs"
sed -n '1,120p' "$SCHEMA_FILE" | cat -n
sed -n '120,240p' "$SCHEMA_FILE" | cat -nRepository: nammayatri/shared-kernel
Length of output: 5640
Sync OpenAPI schema field name with Aeson JSON rename for PodCleanupAck
podCleanupAckJsonOptions renames JSON key errorMessage → error, but PodCleanupAck only deriving (ToSchema), so the generated schema will still document the property as errorMessage (and this propagates through SidecarRefreshResponse.pods). Add an explicit ToSchema PodCleanupAck using matching SchemaOptions (e.g., fromAesonOptions podCleanupAckJsonOptions / equivalent) or stop renaming in JSON.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/mobility-core/src/Kernel/Storage/InMem/Management/Types.hs` around lines
70 - 85, The OpenAPI schema for PodCleanupAck is out-of-sync with the Aeson JSON
rename: podCleanupAckJsonOptions maps errorMessage → "error" for ToJSON/FromJSON
but PodCleanupAck still derives ToSchema (so the schema will expose
"errorMessage"); update this by providing an explicit ToSchema PodCleanupAck
that uses the same Aeson options (e.g., use
genericDeclareUnNamedSchema/fromAesonOptions with podCleanupAckJsonOptions) so
the generated schema matches the runtime JSON keys (affecting PodCleanupAck and
SidecarRefreshResponse.pods), or alternatively remove the rename in
podCleanupAckJsonOptions so schema and JSON already align.
1 participant
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
New Features
Improvements