Automatic hanlding for rehits

In https://github.com/WordPoints/hooks-api/issues/53#issuecomment-167669629 while exploring how to best handle aborting duplicate fires of the same event for the points reactor, it was decided that this is something that should really be developed as a generic piece of the API, not just for the points reactor.

However, how we do that will depend upon whether there might ever be a reactor that would specifically want to handle repeats in a different manner than as would occur if we made this a part of the basic API. To allow for that, we could confine the repeat handling to a child of the reactor class, which reactors could extend if they wanted to handle repeatable events this way.

By a refire or duplicate fire of an event, it is meant that the same event affecting the same object has occurred multiple times. For example, a Post Publish event should only fire once per Post. Firing more than once for the same post would be a refire or repeat fire.

Note: below it was decided that we want to stop not just any refire, but refires that will cause rehits. So an event can fire many times, but each reactor can award only once.

More specifics about how a repeat fire should be identified is discussed in #59. That also has discussion on the fact that not all events require this constraint. Some events are supposed to be repeatable.

This issue is closely tied to #55, and will be possible only with that in place.

For the record, when an event is non-repeatable, we need to abort repeat reverse or spam actions in addition to regular fires.

Retroactive API

It is easy to get confused on exactly how this differs from the hypothetical retroactive API (#1), and how the two might work together. So that is something that we need to clear up before we proceed.

First, it should be noted that the retroactive API's job is to find remnants of past events that were not fired by a reaction, while the hit log/refire detection API's job is to find hits that have already occurred for the reaction. In that sense, the two are sort of opposites.

The refire detection API is actually necessary for the retroactive API to work properly. Otherwise it would end up firing for past events that a reaction has already been fired for. Otherwise a retroactive fire could only occur at the initial creation of a reaction. But with a refire detection API in place, we can not only decide to award points retroactively later, but his action is idempotent. So we could perform the retroactive fire multiple times without any refires occurring. This is valuable if we offer the ability to disable a reaction temporarily (#11) and the user later decides that they want to retroactively award points for fires that occurred during that time (although of course they'd also be awarding for any prior unfired events as well).

In order for this to work correctly, the retroactive API would need to use the hit log/refire detection API to avoid refires, and would also need to log the retroactive hits so that the refire API would know that they occurred. Those are both things that the retroactive API will need to take care of if/when it is implemented, but we should keep them in mind as we build this API so that we don't make it overly difficult.

Where should refire detection occur?

We need to decide at what point refire detection needs to occur. This will help us make the decision as to how to store the data about which args are stateful (#64) so as to best make them available at the point of refire detection.

While we say "refire detection", keep in mind that what we're trying to prevent is duplicate hits. However, we don't have to worry about whether a fire will actually hit in order to detect whether it would be a duplicate if it did hit. So we don't have to wait for a hit to occur before checking whether the fire should be aborted.

Since refire detection should be pretty easy and not resource intensive, it makes sense for us to detect refires as early during the fire routine as possible. (Since actions performed by the extensions may be more expensive.) The question then is what is the earliest point at which we have all of the needed information to detect a refire available to us?

What info is needed?

To answer that question, let's figure out exactly what information we need.

We need to know the event being fired.
What type of fire is taking place (fire, spam, reverse, etc.).
Whether the event has any non-stateful args.
The values of those args.

All of that information is available late in the router, just before the actual fire takes place. So we might conclude that we could detect refires from within the router.

However, if we did that we'd be assuming that all reactions from all of the reactors were there and listening when the original hit occurred. But that's not necessarily true. It's entirely possible that some new reactions have been added that didn't catch that original fire. Also, since whether a fire is a hit is determined per reaction, and we log hits specifically, not all fires (because only the hits matter), we really need to determine this on a per-reaction basis.

So we also need:

The reaction IDs.
The reactor slug (since reaction IDs are only unique to their reactor).
The storage slug (since reaction IDs are only unique to their storage container).

That information isn't available until we are actually in the firer object (at least the way that the current API is built). We could place the handling there, or in the reactors themselves (which I suppose we could actually consider to be "earlier" than after the firer has retrieved the reactions), although this would likely require some modifications to how the firer retrieves the reactions, since we'd need to get only the reactions that are not hit based on the current args (sort of similar to #58?).

Reaction settings changes

Something to consider here is what to do when a reaction's settings change.

Whether a reaction hits the target or not, and if it does, what action is taken, is determined by many of the reactions settings. When these settings change, the behavior of the reaction will change more or less drastically (though usually much less).

Since whether a reaction hits the target on a particular fire of an event is determined by it settings, and its settings an change, it is possible that whether a past hit would actually be counted as a hit today can change to. It is also possible that the results of a past hit would be different than how the reactor would currently respond to the reaction simply because the target or other reaction settings have changed.

Conversely, past misses (which we currently don't intend to log) might also be changed to hits, had the reaction had its modified settings.

Taking into account past hits that might have been misses when we have no way of taking into account past misses that might have been hits would be folly. So I suppose that it is best if we do nothing here at the moment.

Of course, in the future I suppose that it might be possible to retroactively overturn passed hits using the retroactive API (since it could also detect past misses as well).

Although there may be nothing that we can do about all this at the moment, we should at least consider what the consequences of doing nothing would be.

When a reaction's settings change so that it becomes more restrictive, the past hits may have occurred when they otherwise wouldn't have, but this won't stop future fires from hitting that wouldn't have missed anyway.

When a reaction's settings change so that it becomes less restrictive, some past fires which would now hit would have missed. However, they could still hit in the future if another fire of that event occurs, since there is no past hit to prevent a repeat.

When a reaction's settings change so as to have different restrictions, there will be both past hits and misses for which the outcome would now be different. For the past hits that have been logged, they might cause future fires to be called as repeats, but this isn't a problem since a hit has already occurred anyway. For the past misses, there is nothing we can do, although it is possible that the hits could still happen in the future.

The biggest issue is when the target is changed. When this happens, the past hits will block the target from being hit at all. Unlogged past misses will not be a problem.

A possible solution to that would be to disable changing the target in the UI.

Repeat Hits vs Repeat Fires

By checking for repeat hits and not repeat fires, we are allowing the event to fire multiple times, hitting different reactions on different fires. We need to consider the implications of this.

What this does in essence is make it easier for users to game the system. For example, a user could keep changing a post and republishing it to get awarded by different reactions.

To avoid this, what we would have to do is allow the event to fire only once, hit or miss. After that, no matter whether the event was reversed, later fires would be ignored. And if any reactions were added later, they'd have to be taken care of with the retroactive API. (Which makes me wonder: if a reaction isn't retroactive, should it specifically ignore events for existing entities?)

But then reactors which properly handle reversals would no longer work. One fire and one reverse fire would be all that would be allowed. But reactors that reverse the result of a hit when it is reversed should be allowed to fire/hit again after that reversal. This API is therefore not needed for them. Although I suppose that it might still be useful for detecting multiple fires of the same event without a reversal being fired (though that shouldn't normally happen). Perhaps it would make sense to store this data across two tables then: one would contain the data for the event, the other for each particular reaction. The last type of fire that occurred for that event could then be stored in one place, which would simplify things and make it so that different reactions couldn't end up expecting a different kind of fire next for that event (fire vs reverse/spam/etc.). And it would make it so that we could check the status of an event without knowing any details about the reactions.

We could then do very generic stuff like prevent a reverse occurring when a regular fire never occurred. Though I'm not sure that that would be the desired behavior across all reactors. Also, what about when new reactions are added? they still might get a reverse directed at them first before a fire even with such handling in place. So that would still need to be handled on a per-reaction basis. And a properly constructed reactor should already take all of that into account.

Entity Modification without Firing the Event

Another consideration: if it is possible to publish a post, unpublish it, modify it, and then publish it again to receive points; it is inconsistent when we can't just modify a published post and receive points all the same. If we want to allow the one it makes sense to allow the other also. Which brings us back to a fundamental question that each reactor has to answer: is it stateful or cumulative?

We've basically decided that points accumulate, but badges might be stateful. But then, if something is stateful, does it need to be a reactor at all? What it really needs is queries, only queries. It may as well run on-the-fly if it wants to be truly stateful, rather than listening for events. Of course, perhaps there is such a thing as a compromise between the two, one-way valve statefulness, where once a particular state is reached the event fires, and there is no going back even with reversal.

Perhaps it would be better to think of the hypothetical rate/quota API that way: not as preventing hitting, but preventing firing. In other words, it acts as a pressure valve behind which the state of things must accumulate until they reach the threshold, and then a fire occurs. It is sort of like we are stopping fires from occurring, not hits (which is different than the conditions extension). But once that fire does occur, and hits the target, the reactor behaves the same way as it otherwise would, it accumulates. Perhaps it would be better to think of this as a meta-fire and meta-hit or something.

So when a post is modified, how should this be handled by a cumulative reactor? This should be consistent regardless of whether the event is triggered by the change or not. But how can we be alerted to every change to every attribute of an entity and every one of its related entities? It isn't entirely possible to do that because its always possible that they will be modified directly without using the methods provided by WordPress. But that is and edge-case which is far beyond our control. Barring that, what we could do is have a set of actions registered for each entity child that are the actions upon which that relationship/attribute might be modified.

The other option, of course, would be to ignore future fires after a miss occurred. Although that would be consistent, I'm really not sure that it is a good idea. It makes sense that if a post is published without a particular tag and then that tag is later added, the user could be awarded again by the hook being refired.

That really isn't gaming the system. It is just getting what is coming to them. What is gaming the system is when they already got awarded points by some other reactions and are now trying to get points for this reaction to without really publishing another post. Just modifying a single post over and over to rack up points.

The solution to that would be to detect conflicting reactions and not allow hits on future fires after one or more of those reactions has been triggered by the event. This would basically make the non-reversible reactors behave the same as the reversing ones do, except that since the prior hits aren't reversed later ones aren't allowed to occur. (For the reversing reactors, the action of unpublishing the post/reversing the event will cause the prior hits to be reversed, so it is impossible to be rewarded by conflicting reactors.)

Conflicting Reactions

The problem then, is identifying groups of "conflicting" reactions.

Whether two reactions conflict is determined not so much by the reactor settings but by the settings for the extensions (conditions, for example). So it is actually possible that reactions from entirely different reactors can conflict.

I don't think that there is any way for us to reliably detect conflicting reactions without a dedicated API for that purpose. Although there is one other possibility: checking if the reaction(s) that caused the prior hit(s) would still hit under the current fire. If any of them would not, we know that the reactions conflict.

Of course, this all falls apart when a reaction's settings change, meaning that what might have conflicted before wouldn't conflict now, and vice versa. If we saved reaction revisions we'd be able to go back and get the reaction as it was at the time that the hit occurred, but that is something that isn't even on the radar.

On the other hand, what's the worst that could happen because of this? We basically decided above that we don't need to worry about reaction changes as far as detecting repeats goes. Is this really any different? If the reaction changes so as to become conflicted, the user looses out on one event fire. If the reaction changes so as to become unconflicted, they get an event fire that they wouldn't have originally gotten. That is a little different than what happens with repeat detection issues for changing reactions, I guess, but it isn't as if the sky is falling in. It is much simpler than trying to piece together a whole API to do something that our API already does anyway. Frankly, I think that we'd be much better off solving this (if we consider it something that needs to be solved before release) by saving revisions for reactions rather than trying to build a conflict detection API.

The conclusion from the above is that we want to detect repeat hits, not repeat just fires. We'll do #69 to prevent gaming the system.

A possible solution to that would be to disable changing the target in the UI.

This is something we need to consider further.

The conclusion from https://github.com/WordPoints/hooks-api/issues/69#issuecomment-168242717 was that we should do reversals in the points reactor by default, but have the option to disable it for legacy reasons. (In essence, we decided that we shouldn't change how the points hooks behave right now in regard to reversals, and just focus on making the hooks API as flexible as possible.)

This means that we still need to be able to detect rehits, but it gives us a bit of a different perspective on exactly what we're trying to accomplish. First let's talk about scope. We need to provide the basic underlying API to make this possible, but how rehits are handled is something that needs to be handled per-reactor (and even per-reaction). So we don't want to hard-code too much of the implementation into the firer logic. That way if the reactor handles reversals correctly all on its own we can avoid running unnecessary queries on the hit logs, etc.

Extension

I just had the idea of putting the repeat detection code into an extension. This makes a lot of sense since we already have the logic in place that allows an extension to abort a fire and to listen for hits. The only question then is how we would pass the hit ID to the reactor's hit() method? My current mock up of this was passing the hit ID to both the hit() method of the reactor and the should_hit() methods of the extensions. I guess first we could ask whether we really need to do that.

The reason for making the hit ID available was so that any other parts of the code that needs to log hit information wouldn't have to store all of the information in a duplicate location. Instead it could just store the hit ID, and if it needs to be able to run queries, I suppose it could use it's own table and join on the hit logs table.

Should the hit ID be available to the `*hit()` methods?

But I guess we should understand how that would actually be useful.

One place that I was planning to use the hit ID was in the points reactor, logging it as metadata for the points logs. That way when reversals occur, if we could be supplied the ID of the hit that goes along with the reversal, we could easily look up the original fire that way. (Currently we have to use the values of the event args to look up the log matching the original hit, but that is fragile when args are added and removed, I think.)

Another place that having the hit ID might be useful is in the periods extension. It would be helpful if we could just store a hit ID in the periods table along with the period signature, instead of having to store all of the hit info separately. We need to know the ID of the current hit in our after_hit() method, and we'd also need to know the ID of the previous hit in our should_hit() method.

So this would be useful. The question is, how do we make this information available?

Using a fire data object

If we do this as an extension, we don't want to hard-code anything into the logic of the firer object itself—it should really work across firers. Actually, I think that is a very positive aspect of this approach.

The only problem is that no data can currently be passed from one extension to another, or from the extensions to the reactors (or the reactor to the extensions, for that matter).

To solve this, I propose that we introduce a Fire_Data object that gets passed around to all of the extensions and the reactor. Each extension could share information with the others by storing it in this object. (I guess we could event make the event args and reaction validator properties of this object if we wanted to.) The only drawback is that the data wouldn't be available until after that extension's should_hit() method had been run (at the very earliest). It wouldn't be available to any of the extensions that run before that. Sometime we might introduce an extension priority API (#17), but until then, I think we'd just have to register the extensions which need the data after the extensions which provide the data, so that they'll fire in the correct order.

Reassessment

As we've dug into this, our understanding of it has increased and the view on exactly what this ticket is trying to do has evolved and become more clear.

Double hits

At the start it wasn't clear whether we were trying to disallow multiple fires to occur without a reversal in between. I thought that that was part of the issue.

However, as things have moved along, it has become clear that that isn't really an issue at all. For repeatable events, it is possible and expected that multiple fires can occur without any reversals occurring. For non-repeatable events, we came to understand through many of the linked issues that these are tied to the status of a particular entity, and fire and reverse as the status of that entity is toggled. And since an entity either has a particular status (the transition to which will cause the event to fire) or it doesn't (the transition from having the status to this state triggers reverse), fires can't naturally follow one another without a reverse being triggered between them, because an entity can't transition to a particular status when it already has that status—it would have to transition away from that status first, which should trigger the reversal.

And so we see that all of the sorts of events that we currently have will behave properly, as regarding reversals, by nature. The one anomaly, which led to the creation of this ticket, was the Post Publish event. When a post's status was toggled, the reverse event wasn't triggered. This was caused by the event being hooked to post delete only, not to post depublish. We eventually realized that this was a misconfiguration bug, and corrected it. But at the time we did not know that this issue was unique to the Post Publish event, or what was causing it.

Now that that bug is fixed, we no longer have any issues in regard to duplicate hits occurring without intervening reversals during normal operation.

Silent Toggles

The only reason that we would need special handling (as far as these sorts of events go, at least) would be to try to cover the hypothetical case where the status of an entity was toggled in the database without the event being triggered. This is really an extreme edge-case, however.

Hook Misconfigurations

This doesn't mean that there wouldn't be benefits to enforcing the no double-hit rule. I suppose that it could give us cover for cases where we've misconfigured a hook as in the case pointed out above, for example (though that is also an extreme edge-case).

Extension Issues

Then there is the possibility of issues caused by wierdness in extensions. For example, what would happen if we allow hooks to be disabled, and entity statuses were toggled while the hooks were disabled?

Non-reversing Reactors

The focus of much discussion surrounding this ticket had to do with reactors that do not perform reversals. This discussion began with the points reactor, at a time when it handled only some types of reversals, not all. At the time spam fires and reverse fires were separate affairs (this changed in #79), and as such the points reactor handled only spam fires. After the two "types" of reverses were merged into a single reverse firer, the points reactor does technically handle reverses. Around the same time, the decision was also made to have the points reactor handle reverses of all types anyway, which in fact it currently does, though not yet correctly (see #80). However, prior to that it was believed, in essence, that the points reactor should only handle some reverse fires (those classified as "spam"), not all.

While this evolution was taking place, there was confusion as to whether the points reactor was handling only some reversals, or none at all, because the definition of the spam fires as being "reversals" or not wasn't clear. As a result, the reasoning above in this ticket likely confounds the case when a reactor does not handle reversals at all with the case where it handles some reversals but not others.

At that time, handling reversals was an opt-in thing, and required the reactor object to implement a particular interface. However, when the spam and reverse firers were merged, that changed, so that all reactors have the reversal methods defined (and the same for extensions). This, again, is a point of confusion when thinking about this issue.

One of the mixed aims of this ticket then, was to properly handle rehits when the reactor did not handle reversals at all. What this meant at that time, remember, was that the reactor wouldn't have implemented the reverse reactor interface, and so wouldn't have a reversal method. When a reverse action was triggered, the reverse firer would just skip over such a reactor.

This has now changed, and the reversal methods are always called and the reverse logged. I had thought that perhaps due to this change of reversals actually always being handled, the possibility of rehits would no longer exist. However, the underlying issue is still there, because just calling the reversal method will do nothing when it is just a stub not implemented by the reactor. The reversal is logged also, yes, but that does nothing to actually reverse the affect of the original hit, which is what the reversal method of the reactor is expected to do.

I suppose it is actually disingenuous to log the reversal at all, when it hasn't really occurred. We should allow reactors to opt out of supporting reversals without having "fake" reversals fired and duly logged. And so I think it would make sense for us to return to something more like what we had previously, where reactors that handled reversals had to implement an interface, and otherwise were skipped over when a reverse fire occurred. That would be for a separate ticket, however.

And note that this wouldn't actually fix anything, since the lack of a reversal won't stop the firer from allowing another hit to occur. The firer doesn't perform any checks on the hit logs, since some events are repeatable. The only thing that keeps the toggle events from double hitting is that reverse fires naturally happen that cause the effects of the hit to be reversed, when the reactor actually does reverses. If it doesn't, they won't be, and so for such a reactor toggle events would behave just like repeatable events.

Partial-reversing Reactors

There is also a third component to this issue, and that is the case where a reactor really does handle some reversals, but not all reactions that that reactor handles should be reversed. This is a more recent development, or it has only been more recently noted that it is a necessity if we want to import certain points hooks and maintain back-compat (see https://github.com/WordPoints/hooks-api/issues/69#issuecomment-168242717). Basically, some legacy points hooks could have reversals completely disabled. So in order to be able to import those points hooks to the points reactor, we need to be able to disable reversals on a per-reaction basis.

Up to now, our thinking on how best to handle such a case was informed by our focus on rehits and lack of hit logs which could be used to tie reversals to actual hits (see #68). The idea then was to handle the skipping of reversal for particular reactions at the reactor level, and block rehits separately, either at the reactor level, or at the firer level. That later part didn't make a lot of sense, because doing it at the firer level was too generic, but blocking the rehits at the reactor level seemed to specific, and we wanted the solution to be reusable. Thus the decision was made to explore blocking rehits using a custom extension, instead of putting that code into the reactors (see https://github.com/WordPoints/hooks-api/issues/66#issuecomment-171978334).

Of course, we can now see that placing half of the handling in the reactor reversal method (to block certain types of reversals) and half of the code in an extension (to block rehits), is unnecessary. We could have a custom extension do both jobs, if we allowed extensions to abort reverse fires like they do regular fires. The extension would just have to block hits that were repeats by querying the hit logs, and also all reversals.

Where Next?

The biggest question is, which of these three angles should this ticket pursue? All of them? Or perhaps it should be closed in favor of a separate issue for each of them?

Non- to Partial Reversing Reactors

The only thing that keeps the toggle events from double hitting is that reverse fires naturally happen that cause the effects of the hit to be reversed, when the reactor actually does reverses. If it doesn't, they won't be, and so for such a reactor toggle events would behave just like repeatable events.

For non-reversing reactors, why shouldn't toggle events be just like repeatable events? This is something that is really up to the reactor. The same goes for the partial-reversing reactors. It is up to the reactor what behavior it wants to see when no reverse of the hit is triggered.

So it seems that both of these cases would be solved at the reaction level with an extension that blocked rehits and reversals. Actually, the reversal blocking is only necessary for the partial-reversing reactors. So maybe it isn't such a crazy idea to make these to different extensions after all. Or a single extension which had two different settings. Though separate extensions really make more sense then, I guess. Though a single extension would work properly I suppose, since it would never get called for the non-reversing reactors anyway (and even if it did it would do as it was supposed to). But then it seems strange for a reactor that doesn't handle reversals to have all of its reactions use a reversal_blocker extension. And if the two are combined, it would be impossible to use the multihit_blocker part without blocking reversals too, which might not always be desirable. So introducing two extensions allows us to have greater flexibility to configure this at the reaction level. And I suppose it would also be possible to force it at the reactor level, since reactors can save any reaction settings that they want.

I think that is the direction we'll take for this, in new tickets.

Double-hits

The only thing that I think we should pursue as far as double-hits go is this:

Then there is the possibility of issues caused by wierdness in extensions. For example, what would happen if we allow hooks to be disabled, and entity statuses were toggled while the hooks were disabled?

Not so much an issue of double-hits as to how that extension will/should behave.

Of the three parts of this ticket, we have solved the non- and partial-reversing reactor issues with #102 and #107, etc. That leaves only the issue of double-hits, which I am concerned about only in the case that we might sometime want to configure some events differently (as the post publish event used to be, see above). However, probably that should really be done with a different type of event/different types of actions. And that would be for another ticket. So I'm closing out this monstrosity.

Some random notes that were still hanging around, for posterity:

The reason that we need the multi-hit blocker in addition to the reverse blocker is because the fire firer doesn't check the logs to make sure all hits for toggle events have been reversed. Normally toggle events would work so that two fires could never occur without a reverse in between. The only problem is that the reverse might not be a hit, and in that case two fire hits could occur without a reverse hit in between.

The discussion of repeat hits vs repeat fires is moot then, because this is a per-reaction thing. This will only make it possible to "game the system" if all of the reactions are ignoring reversals, which isn't necessarily the case. Only those that are ignoring reversals could be gamed.

WordPoints / hooks-api