POST on task endpoints return LastModifiedBy/On, but GET requests don't include these

[borland]

Team

• [X] I've assigned a team label to this issue

Severity

I expect low severity. The properties are new in 2022 and unlikely customers have taken any dependency on them.

Version

2022.4

Latest Version

I could reproduce the problem in the latest build

What happened?

When doing a GET on a server task via /api/tasks/{id} (or the Space-scoped variant), you get back a serialized TaskResource generated by way of the MapFromServerTaskToTaskResource mapper.

This mapper attempts to populate LastModifiedBy and LastModifiedOn from the Document Metadata, but it gets null, so these two fields are not present in the response.

When doing a POST on /api/tasks/{id}/state to modify the state, you get the same TaskResource generated via the same mapper, but LastModifiedBy and LastModifiedOn are both populated and valid.

This suggests something isn't right on the GET path.

Reproduction

1. Create a server task (any task will do, I used an AdHoc script)
2. Let it run to completion
3. Do a GET on it and save the response JSON
4. Now do a POST to change the task state and compare the response JSON with the GET.

Observe that the GET is missing the LastModifiedOn and LastModifiedBy fields.

I've attached a zip file containing the above when run on my system.

ServerTasks-LastModifiedOn-issue-example.zip

Error and Stacktrace

No response

More Information

No response

Workaround

No response

[matthew-james-octopus]

Understanding of the problem

I've had a look into this but will need some domain context in what I'm observing. Namely:

1. Metadata about a ServerTask (or any document) cannot be accessed between HTTP requests.

   The DocumentExtensionMethods.GetMetadata method relies on storing this data in-memory in a ConditionalWeakTable<TKey, TValue>, with a ServerTask used as the key. According to this class' documentation, ReferenceEquals is used to determine key equality. When we handle a GET api/tasks/{taskId} request and hydrate a new ServerTask from disk, we will always have a new memory address, so querying for related metadata will always fail this scenario.

2. Metadata relating to the last modification is set as a side-effect by the EventStore.

   EventStore.Store sets the 'last modified' metadata properties of a document as a side-effect when storing an event that relates to a document. This means that if we update a ServerTask document without raising a corresponding event about it, we will not know when the ServerTask was last modified.

I think this may not be a simple bug fix, but rather implementing expected behaviour for the first time. By design, there appears to be no way to query these last modification properties in a standalone GET api/tasks/{taskId} request.

Potential solutions

We could, depending on the functionality we want:

1. Continue to cache this information: but choose a caching system that reliably returns the correct metadata for any two documents with the same ID. Multi-node scenarios should be considered, however, so non-distributed in-memory caching is not a good option.
2. Persist this information to disk: since we already appear to store the DateTimeOffset LastModified of any Entity, we could also store who modified a document last.

---

Outstanding questions

1. Why is the LastModifiedBy property not already stored in a ServerTask (or any Entity), and is only held in-memory in associated metadata?
2. Should the EventStore have the responsibility of updating this 'last modification' data, or should this be moved to classes that extend Entity? E.g. in this case, ServerTask could handle updating its LastModified (and new LastModifiedBy) property when its MarkAs method is called.

[matthew-james-octopus]

I wanted to know whether removing the metadata setting in EventStore.Store methods would make any tests fail for valid reasons. I made the following modification to the Store methods:

if (model is IDocument<TKey> doc)
{
    var meta = doc.GetMetadata();
-   meta.LastModifiedBy = octopusPrincipalProvider.Username;
-   meta.LastModifiedOn = clock.GetUtcTime();
+   // meta.LastModifiedBy = octopusPrincipalProvider.Username;
+   // meta.LastModifiedOn = clock.GetUtcTime();
}

Then, according to my understanding of this build, the only tests that break are those that use Assent tests. Since these are strict snapshot tests, and not making strict assertions about particular behaviour, these can probably be ignored.

So, there don't appear to be any defined behaviours expected surrounding querying a ServerTask's last modification details. Due to this, it's Core Platform Scale's opinion that this shouldn't be treated as a bug fix and should instead be seen as an opportunity to define this expected behaviour for the first time.