feat(openfga): add creating, updating, and deleting OpenFGA event handlers #934
Conversation
… struct for app handler arguments
maxwellL21
changed the title
michalkrzyz
left a comment
michalkrzyz
left a comment
There was a problem hiding this comment.
Conditional in tests has to be avoided.
| } | ||
|
|
||
| for _, rel := range rlist { | ||
| authz.AddRelation(rel) |
There was a problem hiding this comment.
remove for loop and avoid using lists if that is not necessary
| }) | ||
|
|
||
| if updateEvent, ok := e.(*UpdateComponentVersionEvent); ok { | ||
| versionId := strconv.FormatInt(updateEvent.ComponentVersion.Id, 10) |
|
|
||
| if updateEvent, ok := e.(*UpdateComponentVersionEvent); ok { | ||
| versionId := strconv.FormatInt(updateEvent.ComponentVersion.Id, 10) | ||
| newComponentId := strconv.FormatInt(updateEvent.ComponentVersion.ComponentId, 10) |
internal/app/component_version/component_version_handler_test.go
Outdated
Show resolved
Hide resolved
internal/app/component_version/component_version_handler_test.go
Outdated
Show resolved
Hide resolved
| UserType: "user", | ||
| UserId: "userID", | ||
| ObjectId: objectId, | ||
| ObjectType: "component_version", |
There was a problem hiding this comment.
repeated code, please use consts to avoid inline duplications, which are source of maintenance problems and slips.
| }, | ||
| } | ||
|
|
||
| for _, rel := range relations { |
There was a problem hiding this comment.
if you want to keep for loop everywhere to follow some logic iterating through relocations, please create helper function for that and use it everywhere (even with single elements)
| UserType: "component_instance", | ||
| Relation: "component_instance", | ||
| ObjectType: "issue_match", | ||
| ObjectId: openfga.ObjectId(issueMatchId), |
There was a problem hiding this comment.
use consts instead of inline strings
| ServiceID: serviceFake.Id, | ||
| OwnerID: ownerFake.Id, | ||
| } | ||
| serviceId := openfga.ObjectId(strconv.FormatInt(removeEvent.ServiceID, 10)) |
There was a problem hiding this comment.
As far as I see the ObjectId has int parameter, but in code mostly we are dealing with string Id, so we convert it all the time and we add unnecessary complexity to the code. consider to add extra function like ObjectId with string parameter (or change the function itself) and hide itoa convert in there. This way we will have great improvement, removing a lot of duplicated code.
| }, | ||
| } | ||
|
|
||
| for _, rel := range rlist { |
There was a problem hiding this comment.
Maybe we could change AddRelocation declaration to accept list parameters instead of doing it all the time in the code. Reducing complexity of for loop is worth it.
| } | ||
|
|
||
| for _, rel := range relations { | ||
| authz.AddRelation(rel) |
There was a problem hiding this comment.
Consider to create AddRelocations, you may keep AddRelocation calling AddRelocations with single list element, but this will remove many duplications.
| objectParts := strings.SplitN(tuple.Key.Object, ":", 2) | ||
|
|
||
| for _, r := range filters { | ||
| if r.UserType != "" && (len(userParts) < 1 || userParts[0] != string(r.UserType)) { |
There was a problem hiding this comment.
enclose conditionals in helper functions with meaningful name. Anyway why did you create so many if statements with the same body. Please use single if statement, group queries with helper conditional checks. If this is copied behavior from some other part of the code, then please create comment linked refactoring ticket in backlog, we will try to do the best to fix the situation, because it makes the code stink more and more and violates scout rule for sure.
| func (a *Authz) RemoveRelationBulk(r []RelationInput) error { | ||
| tuples, err := a.ListRelations(r) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
I think we could wrap the error, because ListRelocations error might be too meaningless for the user. Especially when this is exported function.
| return err | ||
| } | ||
|
|
||
| func (a *Authz) UpdateRelation(r RelationInput, u RelationInput) error { |
There was a problem hiding this comment.
Please use meaningful names in parameters. I do not understand what is r and u meaning without reading of the function body, this is the sign of poor design.
| }) | ||
|
|
||
| err := a.RemoveRelationBulk([]RelationInput{r}) | ||
| if err != nil { |
There was a problem hiding this comment.
IMHO This code is wrong by design if RemoveRelocationBulk will return error, we will still call AddRelocation which may hide an error returning nil, so we will return nil even if some function will fail inside. Is it what we would like to have in here?
Anyway is the else part correct? I see 'Added relocation tuple....' log in the case of successful RemoveRelocation. Looks like action-based slip/copy paste error.
… in authz unit tests
Description
This PR adds the create, update, and delete tuples event handlers for events such as CreateComponentInstance and DeleteSupportGroup. Please review full list of tuples implemented by handlers listed in the document here
As part of the OpenFGA implementation, when Heureka entities are modified, the relation tuples in OpenFGA should be similarly updated using the event registry logic.
What type of PR is this?
Related Tickets & Documents
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist