Conversation
|
Review changes with |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughSuperpositionProvider module now exposes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clients/haskell/superposition-bindings/lib/FFI/Superposition.hs (2)
4-4: Missing exports for new functions and types.
getApplicableVariants,GetApplicableVariantsParams, anddefaultApplicableVariantsParamsare defined but not exported from the module. If these are intended for external use, add them to the export list; otherwise, this is dead code.Proposed fix
-module FFI.Superposition (getResolvedConfig, ResolveConfigParams (..), defaultResolveParams, MergeStrategy (..)) where +module FFI.Superposition (getResolvedConfig, ResolveConfigParams (..), defaultResolveParams, MergeStrategy (..), getApplicableVariants, GetApplicableVariantsParams (..), defaultApplicableVariantsParams) where
102-135: Critical memory management issues: allocator mismatch and memory leak.Two concerns:
Allocator mismatch:
newCStringandcallocBytesallocate memory using the C runtime allocator, butfree_string(callingcore_free_string) likely uses Rust's allocator. Mixing allocators causes undefined behavior.Memory leak: The
CStringreturned byget_resolved_configis allocated by the C/Rust library but is only peeked withpeekCAString—the original C string is never freed.Proposed fix
+import Foreign.Marshal.Alloc (free) + getResolvedConfig :: ResolveConfigParams -> IO (Either String String) getResolvedConfig params = do ebuf <- callocBytes 256 let ResolveConfigParams {..} = params newOrNull = maybe (pure nullPtr) newCString - freeNonNull p = when (p /= nullPtr) (free_string p) + freeHaskell p = when (p /= nullPtr) (free p) peekMaybe p | p /= nullPtr = Just <$> peekCAString p | otherwise = pure Nothing dc <- newOrNull defaultConfig ctx <- newOrNull context ovrs <- newOrNull overrides di <- newOrNull dimensionInfo qry <- newOrNull query mergeS <- newCString $ show $ fromMaybe Merge mergeStrategy pfltr <- newOrNull prefixFilter exp <- newOrNull experimentation - res <- - peekMaybe - =<< get_resolved_config + resPtr <- get_resolved_config dc ctx ovrs di qry mergeS pfltr exp ebuf + res <- peekMaybe resPtr + when (resPtr /= nullPtr) (free_string resPtr) -- Free C-allocated result err <- peekCAString ebuf - traverse_ freeNonNull [dc, ctx, ovrs, di, qry, mergeS, pfltr, exp, ebuf] + traverse_ freeHaskell [dc, ctx, ovrs, di, qry, mergeS, pfltr, exp, ebuf] -- Free Haskell-allocated pure $ case (res, err) of (Just cfg, []) -> Right cfg (Nothing, []) -> Left "null pointer returned" _ -> Left err
🧹 Nitpick comments (1)
clients/haskell/open-feature-provider/lib/Data/OpenFeature/SuperpositionProvider.hs (1)
115-116: Consider extracting duplicate helper to module level.
getTaskOutputis defined identically in bothevalConfig(line 116) andgetStatus(line 170). Consider extracting it as a top-level helper to reduce duplication.Proposed refactor
Add near other helper definitions:
getTaskOutput :: DynRefreshTask a -> IO (Maybe a) getTaskOutput (DynRefreshTask t) = getCurrent tThen remove the local
wheredefinitions in both functions.Also applies to: 169-170
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clients/haskell/open-feature-provider/lib/Data/OpenFeature/SuperpositionProvider.hsclients/haskell/superposition-bindings/lib/FFI/Superposition.hs
🔇 Additional comments (5)
clients/haskell/open-feature-provider/lib/Data/OpenFeature/SuperpositionProvider.hs (5)
69-84: LGTM!The prefix parameter is properly added and threaded through to the FFI binding.
98-116: LGTM!The
evalConfigfunction properly composes config and experiments, handles the case when config is unavailable, and maps FFI/parse errors to appropriateEvaluationErrortypes.
118-134: LGTM!Refactoring to use
evalConfigsimplifies the code. The key extraction logic with proper error handling forFlagNotFoundis correct.
153-158: LGTM!Clean API design: exposing
resolveFullConfigpublicly while keepingevalConfigas an internal implementation detail.
164-170: Correctly fixes the status check per PR objectives.The implementation now properly queries the config refresh task's current state to determine provider readiness, rather than using an incorrect status indicator.
| getApplicableVariants :: | ||
| GetApplicableVariantsParams -> | ||
| IO (Either String String) | ||
| getApplicableVariants GetApplicableVariantsParams {..} = do | ||
| ebuf <- callocBytes 256 | ||
| let newOrNull = maybe (pure nullPtr) newCString | ||
| freeNonNull p = when (p /= nullPtr) (free_string p) | ||
| peekMaybe p | p /= nullPtr = Just <$> peekCAString p | ||
| | otherwise = pure Nothing | ||
| exps <- newOrNull experiments | ||
| expGrps <- newOrNull experimentGroups | ||
| dims <- newOrNull dimensions | ||
| qry <- newOrNull queryData | ||
| tkey <- newOrNull targetingKey | ||
| pfltr <- newOrNull filterPrefixes | ||
| res <- | ||
| peekMaybe | ||
| =<< get_applicable_variants | ||
| exps | ||
| expGrps | ||
| dims | ||
| qry | ||
| tkey | ||
| pfltr | ||
| ebuf | ||
| err <- peekCAString ebuf | ||
| traverse_ freeNonNull [exps, expGrps, dims, qry, tkey, pfltr, ebuf] | ||
| pure $ case (res, err) of | ||
| (Just vars, []) -> Right vars | ||
| (Nothing, []) -> Left "null pointer returned" | ||
| _ -> Left err |
There was a problem hiding this comment.
Same memory management issues as getResolvedConfig.
This function has the same allocator mismatch and memory leak issues. Apply the equivalent fix: use Haskell's free for strings allocated with newCString/callocBytes, and call free_string on the result pointer returned by get_applicable_variants.
There was a problem hiding this comment.
@Datron - this came in my toml PR as well - should we just address this ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| foreign import capi "superposition_core.h core_free_string" | ||
| free_string :: CString -> IO () | ||
|
|
||
| foreign import capi "superposition_core.h core_get_applicable_variants" |
There was a problem hiding this comment.
@Datron - why do we rename the generated core.h file to superposition_core.h in our builds ? Can't we use it as it is ?
There was a problem hiding this comment.
@knutties some languages expect the binary name and the header file name to be the same - that is why we do the rename
| runLogger $ Log.logError ("An error occured while resolving a key." Log.:# lctx) | ||
| pure $ Left $ e | ||
|
|
||
| resolveFullConfig :: |
There was a problem hiding this comment.
Why two functions with exactly same signature @Datron - resolveFullConfig and evalConfig - or am I missing some Haskell basics here?
There was a problem hiding this comment.
@knutties keeping the naming convention the same as the spec - its just that resolveFullConfig and evalConfig does the same thing. This is the same as the rust provider PR I have raised
|
Closing this since breeze team currently can't even use these changes |
2 participants
Problem
the status returned by the haskell provider is wrong
Solution
Get the correct status by checking the config data stored in the polling thread
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.