Skip to content

bug: have a type for combined ids, fix #653#664

Merged
MichaelMure merged 2 commits intomasterfrom
combined-id-rework
Aug 23, 2022
Merged

bug: have a type for combined ids, fix #653#664
MichaelMure merged 2 commits intomasterfrom
combined-id-rework

Conversation

@MichaelMure
Copy link
Copy Markdown
Contributor

fix #653

@MichaelMure
Copy link
Copy Markdown
Contributor Author

FYI @smoyer64

To deal properly with that PR, there is a bunch of questions that need a good answer (which is why I guess I've been stuck for so long):

  • should CombinedId also carry an entity type identifier? If so, how?

I guess? #843 could benefit from this to be able to link to any entity (as long as it does implement the required interface). But it might also be a case of too much generalization and ProjectBoard should just carry that identifier as part of the operation linking to the entity. It's probable that linking to "any entity" won't be such a thing really, so why put that in CombinedId? Or maybe having that information as part of the identifier would help in the long run for sharding or whatever else data routing purpose? If we introduce it now, we would avoid having three different king of identifier (id, id+entity_id, id+entity_id+entity_type).

I'm quite unsure here.

If we do want that, I suppose encoding would be either the first byte, or some kind of varint like multihash does.

  1. should CombinedId be "destructive" and not remember the original Ids?

When we construct a CombinedId, we could keep around the original primary/secondary Ids and have them available while we are still working on the entity. When we resolve an entity with a CombinedId, we should have both ids available during the process (?). So, should CombinedId be the resulting interleaved string that encode only two prefixes, or should we keep around the original Ids?

I'm kinda leaning on not keeping them around (as a way to avoid having the requirement of always having the original IDs when building one), but I don't have strong argument.

  1. should types addressed with a CombinedId (like Comment) carry both Id and CombinedId?

Kinda goes hand to hand with the previous question. I suppose this one is easier to answer as I can try to only have a CombinedId and see where that breaks, but that sort of influence the previous choice anyway.

  1. should graphql have a single ID or also get typed ID + CombinedID? Also, should some types carry both?

gqlgen is capable to bind ID with multiple types, so those details could be hidden in the API. But we could also not do that and also type properly there.

I'm leaning on typing properly, but that's also a breaking change.

@MichaelMure
Copy link
Copy Markdown
Contributor Author

I think I got on top of it with the following choices:

  • should CombinedId also carry an entity type identifier? If so, how?

No. It kinda looked like premature thinking. ProjectBoard should most likely do its own thing and would not really benefit from the identifier carrying an entity type.

  • should CombinedId be "destructive" and not remember the original Ids?

Yes. It makes things simpler without edge cases.

  • should types addressed with a CombinedId (like Comment) carry both Id and CombinedId?

No. It's not needed. The only counter example is Comment where also knowing the operation Id is important to edit the comment, so having that as part of the Comment avoid resolving/searching. For this case I added it as TargetId(), clearly different than an Id of the Comment itself.

  • should graphql have a single ID or also get typed ID + CombinedID? Also, should some types carry both?

I went for typed ids. Looks like it works well.

@MichaelMure
Copy link
Copy Markdown
Contributor Author

Missing here is to adjust a bit the webUI graphql queries.

@smoyer64
Copy link
Copy Markdown
Collaborator

We've talked about these CombinedIds several times and somehow I still didn't recognize that #850 was a symptom of the existing Ids. If CombinedId is destructive, should it be DerivedId? (or something else - e.g. Full name is Given name + Surname in some form but I can generally go back if I know how the latter two were combined.

@MichaelMure MichaelMure marked this pull request as ready for review August 23, 2022 12:49
@MichaelMure
Copy link
Copy Markdown
Contributor Author

If CombinedId is destructive, should it be DerivedId?

To me DerivedId sound like one thing morphed into another (as in crypto: data --> hash, or data --> key), while here we have two things merged into one.

@MichaelMure
Copy link
Copy Markdown
Contributor Author

A year+ later, I think this is finally ready!

@MichaelMure MichaelMure merged commit 5a70e8b into master Aug 23, 2022
@MichaelMure MichaelMure deleted the combined-id-rework branch August 23, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editing comments (in TermUI and WebUI) has no effect

2 participants