Refactor gets

[guillemcordoba]

Summary of changes:

• Updated syn and pretty-please dependencies, this is needed to handle the new rust let Some(_) = my_option else {}; statements.
• Disambiguated the get_entry_type_name getters:
  • Previously, we only had a get_entry_type_name(action_hash: ActionHash) -> ExternResult<Option<Record>> that:
    • If the entry was deleted, returned None
    • Otherwise, would return the latest update for the entry
  • This is bad for a couple of reasons:
    • Whenever there is an update to the entry, there is actually no way for us to get the original action, so things like the creation timestamp are not accessible anymore, there are no calls that retrieve it
    • Once the entry is deleted, there is no way to see it again, if you want to implement an "archived" list of entries
    • There is no get_all_updates getter, so the UI cannot show a list of updates
  • So this PR disambiguates that single get into multiple ones:
    • If the entry can be updated:
      • get_latest_entry_type_name(action_hash: ActionHash) -> ExternResult<Option<Record>>: gets the latest update for the entry, returns the entry even if it's deleted
      • get_all_entry_type_name_revisions(action_hash: ActionHash) -> ExternResult<Vec<Record>>: gets the list of updates for the entry, returns them even if it's deleted
      • get_original_entry_type_name(action_hash: ActionHash) -> ExternResult<Option<Record>>: gets the create record for the entry, returns the entry even if it's deleted
    • If the entry cannot be updated:
      • get_entry_type_name(action_hash: ActionHash) -> ExternResult<Option<Record>>: gets the create record for the entry, returns the entry even if it's deleted
    • If the entry can be deleted:
      • get_deletes_for_entry_type(action_hash: ActionHash) -> ExternResult<Vec<SignedActionHash>>: returns the array of Delete actions for the entry, allowing the UI to display information like "it was deleted 4 minutes ago", that was previously not accessible

[mattyg]

I see where you're coming from. I wonder if there should also be a get_oldest_delete_for_entry_type(action_hash: ActionHash) mirroring the function that gives an opinionated "latest" update.

Anyway since this doesn't actually remove the existing function and just renames it + adds additional ones, I'm fine with it. I'd have to play around with it to get a feel for it anyway.

[guillemcordoba]

Mmm interesting... Did you find yourself needing that function at some point?

[mattyg]

Mmm interesting... Did you find yourself needing that function at some point?

Yeah I just think the standard use case is only caring about the oldest delete because that's what you would treat as the "canonical" timestamp and delete author. Having multiple deletes that are all meaningful seems like an edge use case.

It also feels weird to me that you're including an opinionated way to define the "latest" update, while not also providing an opinionated way to define the "latest" delete -- either have both or neither.

[guillemcordoba]

Hum my reasoning was performance: not having an opinionated get_latest would hurt performance:

• In the case where each new update is not linked from the original create action, the UI would need to do a separate zome call for each update.
• In the case where there is a link from the create to each new update, not having an opinionated get_latest means not taking advantadge of the linking, so obviously it's a performance problem.

I actually feel that this default will need to be changed or at least constrained by most applications, but it's a good starting template. What I mean is that get_latest_x is easily attackable by adding an update on a branch that you like with a later timestamp than the other ones. So either you add validation so that only certain agents can do updates (in which you don't eliminate the problem, but at least you constraint it), or you come up with clever update branch selection / conflict resolution mechanisms... both of which are out of the scope for the scaffolding, at least for now.

However, in the case of delete the unopinionated call is just as performant as the opinionated one (a single get_details), and I defaulted to returning all the available information without sacrificing on performance.

I see what you mean in that having a vector of deletes be relevant is an edge case. I'm thinking about partition situations, where you might as well display "it was deleted by these two agents", or race conditions... What do you think?

[guillemcordoba]

@ThetaSinner @mattyg I'd like to get this and a couple other PRs merged as other work I'm doing depends on them. Could I get a review / go ahead?

[mattyg]

Hum my reasoning was performance:

I'm definitely in favor of having an opinionated get_latest for both updates and deletes -- not just for performance but also for dev ergonomics. For ergonomics its valuable to have coherent consistent functions.

However, in the case of delete the unopinionated call is just as performant as the opinionated one (a single get_details), and I defaulted to returning all the available information without sacrificing on performance.

Except that in the typical case you then have to filter the deletes on the front-end.

I defaulted to returning all the available information without sacrificing on performance.

But you're not returning the updates from the get_details, right? That's because you're also trying to make a meaningful useful, constrained function that gets exposed to the dev.

I see what you mean in that having a vector of deletes be relevant is an edge case. I'm thinking about partition situations, where you might as well display "it was deleted by these two agents", or race conditions... What do you think?

IMO it is the atypical case where redundant deletes is meaningful information to display in the the app. In a typical case, all I would care about is that the information has been deleted by someone with authority to do so at a timestamp.

I'm in favor of renaming get_deletes_for_entry_type to get_all_deletes_for_entry_type, and adding an additional function get_oldest_delete_for_entry_type.

[guillemcordoba]

Yeap! Your proposal makes perfect sense @mattyg , and it aligns with the way that updates work. So I'll make that change and then we can merge this. Thanks for your feedback.

[guillemcordoba]

@ThetaSinner this is ready to be merged AFAICT. Can I get get a review? Also, what's the policy on updating the holochain-0.x branches? I only need this to be included in the holochain-0.2 one, and maybe we don't want to backport it to 0.1 since it'd be a breaking change to people with already scaffolded apps?

[ThetaSinner]

Looks great!

[guillemcordoba]

Great, can I merge it then @ThetaSinner ?

[ThetaSinner]

Yes please :) and for the back-porting question. This is targeting the right branch for now. It should just be back-ported to develop-0.2 once it's merged. But that can be done in a batch because there's quite a bit outstanding to back-port so I'm happy to do that if that's helpful :)

[guillemcordoba]

Amazing, thanks! One less to go.