Rule enabled by default breaks invariants
- Dominant language
- Scala
- Stars
- 6.8k
- Forks
- 1.2k
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 2
Description
I merged #1024 but then realized shortly after there was a corner case we missed that both @markusthoemmes and I were concerned about - we were looking in the wrong place. I couldn't think of a way to test the corner case without significant refactoring but maybe there is a way. Let me explain:
The order of operations before the merge was:
```
1a. write the trigger to db with rule inactive state
2a. write the rule to db
```
If either `1a` or `2a` failed, the rule is essentially inactive. If `2a` failed, the rule won't match on a trigger firing, consistent behavior with rule not existing.
After the merge,
```
1b. write the trigger to db with rule in active state
2b. write the rule to db
```
If `1b` succeeds but `2b` fails, the trigger will now match the rule even though it doesn't exist. Inconsistent behavior.
Instead the operations should be:
```
1a. as before
2a. as before
3. in WhiskRulesApi.create `postProcess`, enable the rule by updating the trigger.
```
If `3` fails, the API should fail with a custom rejection:
```
+ // the rule was created but the trigger update failed so without an explicit error the client
+ // may receive a message that lacks context; if the trigger update failed, then the client must
+ // enable the rule explicitly
+ info(this, s"[PUT] failed to update trigger after rule creation: ${t.getMessage}")
+ RejectRequest(Conflict, "Rule was created but remains inactive (auto-enable failed, try again).")
```
1a, 2a and 3 is essentially the equivalent of `wsk rule create & wsk rule enable`. To this, it may be worthwhile to lift such _macro_ operations into a new layer in the controller which uses _micro_ operations so that the logic for the micro operations remains simpler and easier to reason against. Another use of the _macro_ layer: renaming entities.
One way to test this case, is to create the rule, then delete it explicitly from the db and call the above mentioned post process method directly to confirm that the response is `Conflict`.
Note that the reason we have an explicit rule record is that we're using a document store. For example, if the trigger recorded the rule details and a map view constructed the rules, there is a period during which the rule is not known to exist (because either the view is not up to date or hasn't reached eventual consistency across nodes in a cluster).
Contributor guide
Assessment
This issue has not been assessed yet.