Storage promotion/demotion and legacy object unpinning for entity state API

[stalgiag]

This PR adds support for file promotion/demotion & legacy object unpinning with the entity state API.

I am new to Elixir and Phoenix so apologies in advance if I am missing something about preferred patterns. I added two units tests. I believe that most of this behavior has coverage in the existing storage tests.

PR #6092 in the client repo is dependent on these changes.

[johnshaughnessy]

delete_entity_state needs to deactivate the associated OwnedFile, if there is one. The way I proposed doing this in a comment in the PR for the client-side changes was for the client to provide the file_id to the call. This should work (and is how the current unpin behavior works), but upon further reflection it's suspicious that reticulum would need the client to provide this information at all, rather than it keeping track of the association on its own. In other words, maybe rows in the entity_state table should have a nullable foreign key into the OwnedFile table.

[johnshaughnessy]

My guess is that this pattern is not idiomatic elixir, because your intent is something like "If there's a file_id, try to promote/activate it (potentially returning an error), then broadcast. If there's not a file_id, just broadcast." And it feels strange to stick the "then broadcast" part inside handle_file_promotion.

But I don't know elixir well enough to know how experts tend to write this (common) case. No need to change it, but if you find a better way, please let me know because I'll be curious to learn it.

One idea is that maybe_promote_file returns :ok or {:error, reason}, and the file_id: nil case is just :ok. Then the calling code uses if or with.

[johnshaughnessy]

edit: Approved by mistake -- still need to review latest commits

[johnshaughnessy]

(Sorry to muddy the history. I approved by mistake. Will followup with a proper review.)

[stalgiag]

Ready for review again. I took your advice and went with a maybe_promote_file pattern which I like much better. Also added the code that handles unpinning of legacy objects and demotion (set inactive).

[johnshaughnessy]

This handler (and the following one) looks suspicious.

If for some reason the delete_entity_state request does not result in the entity state being deleted (and the associated OwnedFile becoming inactive), this function should reply_error with a reason. The reasons we should catch are:

• If the socket is unauthorized,
• If delete_entity fails (e.g. the entity doesn't exist),
• (Maybe?) If the OwnedFile doesn't exist

Also, I think we need to handle the RoomObject records slightly differently.

On the client, when we load the legacy room objects, we synthesize messages for them so that the client can act as if they are entity create/update messages. (See loadLegacyRoomObjects and messageForLegacyRoomObject in the client.)

We would like to add to this flow a step where a client, when it synthesizes this message, also calls save_entity_state so that that data is migrated to the new EntityState storage (non destructively).

Therefore, when reticulum receives an delete_entity_state directive, there are two (valid) states we should be concerned with:

1. There is both an EntityState and a RoomObject for this nid. We need to delete both.
2. There is only an EntityState for this nid. We only need to delete the entity state.

Deleting a non-existant RoomObject is probably no trouble, so trying to delete both each time is probably a fine strategy (knowing that many times, the RoomObject won't exist).

And, as you have done here -- if there is a file_id in the payload, we need to mark the associated OwnedFile as inactive. To avoid duplicating code, might consider having a single handle_in("delete_entity_state", %{"nid" => nid } = payload ... , and if payload has a file_id, call set_inactive. But that's more a matter of style, not correctness.