In #1195, I had to skip a few tests because the tests were resulting in more entity.error events than before. This was confusing to me, because I thought that a single submission version could result in at most one entity.error.
However, looking at Entities._processSubmissionEvent(), I don't actually see any logic along those lines. The function will return early if the submission has already resulted in an entity version, but it doesn't check whether the submission has already resulted in an entity.error. I don't think there's even an easy way to do so, as we don't have an index on submissionDefId in the audits table.
A couple of the tests that failed referenced getodk/central#547. Reading that issue, it sounds like we decided to focus on the processing of pending submissions when the approvalRequired flag is toggled. That mechanism won't reprocess a submission if it has already resulted in an entity.error. I think that was intended to prevent unexpected reprocessing of entity updates, but it also has the effect of preventing multiple entity.error events.
However, even though multiple entity.error events aren't possible through that mechanism, there are other ways that a single submission version can result in multiple entity.error events. It can happen if an invalid submission is approved multiple times:
Create an entity list that requires submission approval.
Create a submission that creates an entity in the entity list, but that doesn't specify an entity label. This is invalid, because a label is required.
Approve the submission. Observe that there is an entity.error.
Change the review state to something else.
Without editing the submission, approve it again. Observe that there is a second entity.error.
I don't think we necessarily need to lock down this behavior. Maybe it's actually sort of nice to be able to force an individual submission to be reprocessed when the submission resulted in an error. However, while reviewing #1196, @ktuite and I discussed adding tests that document that behavior. That's what this issue is for.
There's another case that's interesting to highlight. Whenever a submission is created, it is at least partially processed for entities. For example, if the submission specifies an entity create, and submission approval is required, then the submission.create event won't result in a new entity — the submission won't be fully processed — but it will still be partially processed. If there's then a submission.update event to approve the submission, the submission will be reprocessed, this time fully. The important thing is that even when a submission is partially processed, it can result in an entity.error for certain basic errors. These are the errors that are checked before Entities._createEntity() and/or Entities._updateEntity() are run. For example, if an entity create is attempted using a form that only specifies entity updates, that will result in an entity.error, even if it's a submission.create event and submission approval is required:
Create an entity list that requires submission approval.
Publish a form that updates but does not create entities in the entity list.
Create a submission to the form that tries to create an entity in the entity list. Observe that there is an entity.error even though the submission wasn't approved. That's because this check is completed early in Entities._processSubmissionEvent().
Approve the submission. Observe that there is a second entity.error.
Because these checks are completed early, a submission approval can result in an entity.error even if the entity list doesn't require submission approval. You can see that by repeating the steps above for an entity list that doesn't require submission approval.
The reason why #1195 resulted in more entity.error events is that the validation of the UUID was moved to earlier in Entities._processSubmissionEvent(). The UUID is needed to lock the entity, and the entity is locked before Entities._createEntity() or Entities._updateEntity() is run. Previously, processing a submission.create event would never reach the UUID validation if the entity list required submission approval, given how partial processing works. However, now that the UUID validation happens earlier, at the same time as some other basic checks, a submission.create event can result in an entity.error for an invalid UUID even if submission approval is required.
We could avoid that, reverting to the previous behavior. To do so, we would still attempt to parse the UUID early on in order to lock the entity, but if parsing failed, we would suppress the error. That would allow Entities._createEntity() and Entities._updateEntity() to operate as they did before, failing on an invalid UUID only if they actually reached the validation and didn't return early. However, that would be extra logic, and I don't think it's needed. I think it's OK that even partially processing a submission for entities can result in an entity.error for certain basic errors, and I think it's fine for UUID validation to be one of those basic errors.
In #1195, I had to skip a few tests because the tests were resulting in more
entity.errorevents than before. This was confusing to me, because I thought that a single submission version could result in at most oneentity.error.However, looking at
Entities._processSubmissionEvent(), I don't actually see any logic along those lines. The function will return early if the submission has already resulted in an entity version, but it doesn't check whether the submission has already resulted in anentity.error. I don't think there's even an easy way to do so, as we don't have an index onsubmissionDefIdin theauditstable.A couple of the tests that failed referenced getodk/central#547. Reading that issue, it sounds like we decided to focus on the processing of pending submissions when the
approvalRequiredflag is toggled. That mechanism won't reprocess a submission if it has already resulted in anentity.error. I think that was intended to prevent unexpected reprocessing of entity updates, but it also has the effect of preventing multipleentity.errorevents.However, even though multiple
entity.errorevents aren't possible through that mechanism, there are other ways that a single submission version can result in multipleentity.errorevents. It can happen if an invalid submission is approved multiple times:entity.error.entity.error.I don't think we necessarily need to lock down this behavior. Maybe it's actually sort of nice to be able to force an individual submission to be reprocessed when the submission resulted in an error. However, while reviewing #1196, @ktuite and I discussed adding tests that document that behavior. That's what this issue is for.
There's another case that's interesting to highlight. Whenever a submission is created, it is at least partially processed for entities. For example, if the submission specifies an entity create, and submission approval is required, then the
submission.createevent won't result in a new entity — the submission won't be fully processed — but it will still be partially processed. If there's then asubmission.updateevent to approve the submission, the submission will be reprocessed, this time fully. The important thing is that even when a submission is partially processed, it can result in anentity.errorfor certain basic errors. These are the errors that are checked beforeEntities._createEntity()and/orEntities._updateEntity()are run. For example, if an entity create is attempted using a form that only specifies entity updates, that will result in anentity.error, even if it's asubmission.createevent and submission approval is required:entity.erroreven though the submission wasn't approved. That's because this check is completed early inEntities._processSubmissionEvent().entity.error.Because these checks are completed early, a submission approval can result in an
entity.erroreven if the entity list doesn't require submission approval. You can see that by repeating the steps above for an entity list that doesn't require submission approval.The reason why #1195 resulted in more
entity.errorevents is that the validation of the UUID was moved to earlier inEntities._processSubmissionEvent(). The UUID is needed to lock the entity, and the entity is locked beforeEntities._createEntity()orEntities._updateEntity()is run. Previously, processing asubmission.createevent would never reach the UUID validation if the entity list required submission approval, given how partial processing works. However, now that the UUID validation happens earlier, at the same time as some other basic checks, asubmission.createevent can result in anentity.errorfor an invalid UUID even if submission approval is required.We could avoid that, reverting to the previous behavior. To do so, we would still attempt to parse the UUID early on in order to lock the entity, but if parsing failed, we would suppress the error. That would allow
Entities._createEntity()andEntities._updateEntity()to operate as they did before, failing on an invalid UUID only if they actually reached the validation and didn't return early. However, that would be extra logic, and I don't think it's needed. I think it's OK that even partially processing a submission for entities can result in anentity.errorfor certain basic errors, and I think it's fine for UUID validation to be one of those basic errors.