Cleanup the core messages now that v0 is gone #652
Conversation
josephschorr
commented
Jun 10, 2022
josephschorr
commented
- Renames object_and_relation on the RelationTuple to resource_and_relation
- Renames user on the RelationTuple to subject
- Removes the user_oneof, as it is no longer needed
- Changes DirectUsersets into DirectSubjects
- Various small test fixes
- Remove unneeded conversion methods and change to manually written for the remaining
josephschorr
added
area/datastore
github-actions
bot
added
area/api v0
josephschorr
force-pushed
the
cleanup-core
branch
from
a6540fa to
094713d
Compare
There was a problem hiding this comment.
This is a nice cleanup, thank you! ✨ Left some comments about naming.
What's not clear to me is the impact to the proto backward compatibility. Because the type of a field is changed it would look like a breaking change - which seems corroborated by a CI error. What is the plan around that? How can we help folks transition to the newer version without downtime?
| @@ -113,19 +113,15 @@ func (cds *crdbDatastore) Watch(ctx context.Context, afterRevision datastore.Rev | |||
|
|
|||
| oneChange := &core.RelationTupleUpdate{ | |||
There was a problem hiding this comment.
Nit / For a follow up: the direct proto instantiation tends to be rather verbose, would it make sense to create helper methods to help with readability?
There was a problem hiding this comment.
We do have some helper methods defined in various places, so we could in theory grab one, but I like that everything is explicitly named here
| var foundNonTerminalUsersets []*core.ObjectAndRelation | ||
| var foundTerminalUsersets []*core.ObjectAndRelation |
There was a problem hiding this comment.
Did you intended to continue using the Usersets nomenclature in these variables?
There was a problem hiding this comment.
The datastores still do, so I'm unsure whether we should change it (we probably should)
| ObjectAndRelation resource_and_relation = 1 [ (validate.rules).message.required = true ]; | ||
|
|
||
| /** subject is the subject for the tuple */ | ||
| ObjectAndRelation subject = 2 [ (validate.rules).message.required = true ]; |
There was a problem hiding this comment.
This would seem like a deliberate API breakage, and presumably we are OK with that? Would this cause downtime to clients as the server with the new proto definition is rolled out?
| ResourceAndRelation: &core.ObjectAndRelation{}, | ||
| Subject: &core.ObjectAndRelation{}, |
There was a problem hiding this comment.
If a Subject does also have a Relation, which isn't it named SubjectAndRelation, or perhaps why not naming ResourceAndRelation just Relation?
There was a problem hiding this comment.
The real change would be to break the relation out from both, but that is a major refactor, so I left it using ObjectAndRelation. Since I did so, I wanted to be clear that the relation for the operation is in the first ONR
We don't store these protos; they are only used for internal dispatching, so the only real issue with compatibility is during deployment, where dispatches might (briefly) fail. We could back-compat support for that case, or just say "yes, this will have an issue for a short period of time" |
As discussed in standup: understood! I somehow thought those protos where part of the public API. The problem you described perhaps warrants a different conversation on how can we make rolling upgrades that are isolated from each other? Pperhaps |
- Renames object_and_relation on the RelationTuple to resource_and_relation - Renames user on the RelationTuple to subject - Removes the user_oneof, as it is no longer needed - Changes DirectUsersets into DirectSubjects - Various small test fixes - Remove unneeded conversion methods and change to manually written for the remaining
