Extract notification methods in identified object store

[SrikarMannepalli]

Description

Extract notification methods from IdentifiedObjectStore to be able to override sending notifications.

[codecov]

Codecov Report

Merging #77 (0a22430) into main (b99c3ad) will decrease coverage by 0.95%.
The diff coverage is 32.35%.

@@             Coverage Diff              @@
##               main      #77      +/-   ##
============================================
- Coverage     85.15%   84.20%   -0.96%     
- Complexity      305      308       +3     
============================================
  Files            38       38              
  Lines          1495     1519      +24     
  Branches         45       45              
============================================
+ Hits           1273     1279       +6     
- Misses          191      209      +18     
  Partials         31       31              

Flag	Coverage Δ
integration	84.20% <32.35%> (-0.96%)	⬇️
unit	83.37% <32.35%> (-0.98%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...objectstore/ContextuallyIdentifiedObjectStore.java	61.11% <0.00%> (-7.64%)	⬇️
...ertrace/config/objectstore/DefaultObjectStore.java	71.42% <33.33%> (-6.19%)	⬇️
...race/config/objectstore/IdentifiedObjectStore.java	83.33% <40.00%> (-4.38%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b99c3ad...0a22430. Read the comment docs.

[saxenakshitiz]

Also handle delete case. We should update DefaultObjectStore and ContextuallyIdentifiedObjectStore as well.

[aaron-steinfeld]

I think there's some confusion about the flow here, so let's walk through.

Imagine I have an ObjectStore<MyInternalObject>. That means that I'm reading and writing instances of MyInternalObject. Now, imagine MyInternalObject is just a representation of MyApiObject, which is otherwise not exposed to the object store - supporting this scenario is the goal of this enhancement.

So when I call buildValueFromData, we want a Value representation of MyInternalObject - this is what goes to the server. When I call buildValueForChangeEvent, we want a Value representation of MyApiObject - that's what goes into change events (and the default implementation assumes thus that MyInternalObject is the same thing as MyApiObject).

So notice from the signatures MyApiObject doesn't show up anywhere - only a representation of it as a Value. Since that's the class name we want in the events too, we need to expose a way of configuring that.

On upsert, we have previousValue as a Value representation of MyInternalObject - luckily we already have the tools to go back and forth from Value<>MyInternalObject. We need to call something like this.buildDataFromValue(previousValue).map(this::buildValueForChangeEvent)

[aaron-steinfeld]

DefaultObjectStore should be updated, too

[aaron-steinfeld]

Can we add tests for the case where this is a different class than T?

[SrikarMannepalli]

I'm not quite sure I understand. obj.getClass().getName() work for any class right? What am I missing?

[aaron-steinfeld]

Probably poorly phrased ask on my part. It does, but the reason we introduced buildClassNameForChangeEvent is that sometimes T is not the class we care about. I was interested in a test where we've overridden the default behavior (for example where T is MyInternalObject and we want our event to be sending MyApiObject)

[SrikarMannepalli]

Got it. Could you give an example as to how to move forward 😬.

[aaron-steinfeld]

This isn't a complete example, but you'd want to exercise the functionality we've added - with a separate api and persistence class. The tests should work as is with that change, but they also should be extended to verify that the mock change event writer is being called with the expected messages.

[saxenakshitiz]

We should not have this special case. Please note consumer will not be able to handle it.

[aaron-steinfeld]

Consumer may or may not be able to, but at least we give them the option. This maps to the existing behavior (where we put in prevConfig as is, and don't check if it's transformable), but certainly open to other ideas (see #77 (comment) for various options I came up with)

[aaron-steinfeld]

Consumer may or may not be able to, but at least we give them the option. This maps to the existing behavior (where we put in prevConfig as is, and don't check if it's transformable), but certainly open to other ideas (see #77 (comment) for various options I came up with)

[aaron-steinfeld]

Other than the tests I asked for in an earlier comment, the change lgtm. Will let @saxenakshitiz continue the discussion on how we want to handle the invalid previous case though.

[saxenakshitiz]

nit : The test seems to suggest that object persisted in DB is completely different from object being published as part of change event. That is certainly possible. However in most of our usecases, the object being persisted will encapsulate the object being published as part of change event.

[github-actions]

Unit Test Results

21 files  ±0  21 suites  ±0   38s ⏱️ ±0s
78 tests +1  78 ✔️ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 777df10. ± Comparison against base commit b99c3ad.