feat: Added hook data #1506
feat: Added hook data #1506
Conversation
| * Default thread-safe implementation of HookData. | ||
| */ | ||
| public class DefaultHookData implements HookData { | ||
| private final Map<String, Object> data = Collections.synchronizedMap(new HashMap<>()); |
There was a problem hiding this comment.
Do we need a thread safe implementation? If the hook data only lives for the duraion of a flag evalution, there will probably not be concurrent access to this object. IIRC, a flag evaluation happens synchronously.
There was a problem hiding this comment.
Ya I think somebody would have to really try to create a concurrency issue here, because of the nature of hooks. I agree this seems un-needed.
| <T> T get(String key, Class<T> type); | ||
|
|
||
| /** | ||
| * Default implementation using ConcurrentHashMap for thread safety. |
There was a problem hiding this comment.
DefaultHookData uses a Collections.synchronizedMap(new HashMap<>()), not a ConcurrentHashMap
| * Hook data provides a way for hooks to maintain state across their execution stages. | ||
| * Each hook instance gets its own isolated data store. | ||
| */ | ||
| HookData hookData; |
There was a problem hiding this comment.
With this addition, the Constructor of the HookContext changes, which may be a breaking change @toddbaert
There was a problem hiding this comment.
That's certainly a breaking change, yep. @mdxabu you can add new constructors, but you'll have to preserve any old ones. Please add unit tests to confirm this.
|
@mdxabu you will need to force push a commit that is signed off, otherwise the DCO check will not succeed |
Can I squash those commits into one commit, and then I will sign off? |
Yes, you could do that. If you click on the failed DCO action, you schould see a short explanation |
Signed-off-by: mdxabu <[email protected]>
…d ConcurrentHashMap and use a synchronized HashMap instead Signed-off-by: mdxabu <[email protected]>
|
@chrfwow, I force push the commits! Now DCO action runs successfully. |
|
The build currently fails because of formatting issues. You can resolve them automatically by running the spotless plugin |
Signed-off-by: mdxabu <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1506 +/- ##
============================================
+ Coverage 92.82% 93.35% +0.53%
- Complexity 487 492 +5
============================================
Files 46 47 +1
Lines 1170 1189 +19
Branches 103 107 +4
============================================
+ Hits 1086 1110 +24
+ Misses 54 49 -5
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| * Each hook instance gets its own isolated data store that persists only for the duration | ||
| * of a single flag evaluation. | ||
| */ | ||
| public interface HookData { |
There was a problem hiding this comment.
We may be able to use some of the functionality in Structure or it's implementations to do this. We don't need all of those methods, but some wouldn't hurt, I think, and it would reduce duplication and promote consistency.
|
@mdxabu how is it going? We do have an internal dependency on this. Can we support somehow? :) |
|
Yeah, Sure! |
Great, let me know how I can support :) |
|
This PR
Added the Hook Data
Related Issues
Fixes #1472
@beeme1mr, Please review this if there is any problem or if I did anything wrong, ping me!