Object Update Trigger - #40
Conversation
…work Concrete Classes and Descendent Subs
cliffcaseyyet
left a comment
There was a problem hiding this comment.
Very cool, I understand this iteration better than the ones before it. Most of my stuff is organizational or minor but what it's actually doing makes sense.
| - `type`: Type of trigger. `Interaction` is currently wired into RTI subscriptions and statement processing. `ObjectUpdate` is parsed by the config model but object updates currently feed the object cache rather than firing statement triggers directly. | ||
| - `class`: The local HLA interaction class name. For interactions this is matched against the final segment of the RTI interaction class name. | ||
| - `type`: One of `Interaction`, `ObjectCreate`, `ObjectUpdate`, or `ObjectDelete`. | ||
| - `class`: Local HLA interaction or object class name. Matching is exact: an object trigger for `SimEntity` does not also fire for a reflection reported as `Rabbit`. |
There was a problem hiding this comment.
you removed the text about the final segment of the interaction classname. Did that change or get moved somewhere else?
There was a problem hiding this comment.
I think that was just accidentally removed from the docs and is still the case, but we might actually want to use the same convention as Object classes, ie we don't require root but fully qualify below that? dunno
There was a problem hiding this comment.
A conflict is possible in a complex fom with multiple intermediate interaction classes is my point
There was a problem hiding this comment.
Ok, if we want to lean FQN then I'm fine with that. Does that mean most of the examples should be SimEntity.Rabbit or whatever?
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| public class StatementTriggerDispatcher { |
There was a problem hiding this comment.
From prior conversation, I am still not convinced this needs to be a component or even a class, and can probably just go as functions right into the TriggerProcessor. This is pretty low priority on my list though.
There was a problem hiding this comment.
I have no problem with that
| private List<Object> statementPath = List.of(); | ||
| private boolean embedded = false; | ||
| private String objectType; | ||
| private StatementTrigger.Type triggerType; |
There was a problem hiding this comment.
eeeep this is problematic. We were using inheritance to determine between Interaction and Object when we only had two. This property provides the ability for it to get janky, i.e. an InteractionInjectionContext with a OBJECT_DELETE trigger type. It creates ambiguity as to what determines the type. Following the existing convention you should just make two more extensions from InjectionContext (or have three extend ObjectInjectionContext which you make abstract to get the attributes fields). Or at the very least this should be a field in ObjectInjectionContext only and have an enum with UPDATE, CREATE, DELETE only.
On the complete other end you could refactor to not use inheritance at all but then you would need to account for the differences in a single class AND you would lose the intentional overloads in injectionhandler and have to replace them with more logic.
| if (injectionContext instanceof TestInjectionContext) { | ||
| return handler.handleLookup(null, target, injectionContext); | ||
| if (injectionContext instanceof TestInjectionContext testContext) { | ||
| return handler.handleLookup(alias, definitions.get(alias), target, testContext); |
There was a problem hiding this comment.
i'm having trouble figuring out what this is doing, perhaps i didn't understand what it was doing before either and why it needs to be different from the non-validation runs.
There was a problem hiding this comment.
I did catch up to this in another comment but i'll leave it here until that one is figured out.
| : interactionTargetDefinition(hlaClass, target); | ||
| } | ||
|
|
||
| private EventTargetDefinition requireEventTargetDefinition( |
There was a problem hiding this comment.
A good amount of this stuff seems like essentially syntax validation against the FOM, which I would have expected more at the parse stage than the execution stage? It seems to be doing deep FOM inspection of every layer of target validity in the middle of actual processing here. Was there a particular reason that it needed to be processing-time?
There was a problem hiding this comment.
I believe this is called during validation by TriggerProcessor.renderTemplateForValidation() but not during a run, I think that is bypassed, but I can chase it down and get a more satisfying answer.
| return objectCache.findValueResolution(object, attrTarget); | ||
| } | ||
|
|
||
| public ValueResolution handleLookup( |
There was a problem hiding this comment.
ah ok i think i get it now per my other question. It cannot pass a CachedObject as the others do beacuse it doesn't have one.
I think part of why this is weird is that it's chosen to already resolve the cachedobject outside the injection handler instead of having the injectionhandler do that work on receiving the details of an injection. Might be worth considering a refactor there, but perhaps there is another structural reason it needs to be done before the injection is being handled?
Trigger statement emission from object updates
ObjectCreate,ObjectUpdateandObjectDeleteObjectCreateis fired at most once when an object is created, upon the first reflection of requested attributesObjectUpdateis fired on every update (including the first reflection).previousexpression is now available inObjectUpdateto refer to the previous state of the object. This can be used to implement change detection.ObjectDeleteis fired at most once when an object is deleted.triggerexpression inObjectDeletecan access a snapshot of the object's attributes prior to deletion.