Tuple queries with UUID #1335
Merged
Tuple queries with UUID #1335
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- allow resolving tags without hitting the db if a UUID is specified
kian99
force-pushed
the
tuple-queries-with-uuid
branch
from
d2f3ad6 to
5002ccc
Compare
kian99
requested review from
alesstimec,
ale8k,
pkulik0,
SimoneDutto and
babakks
ale8k
reviewed
Sep 2, 2024
| if tag.Relation.String() != "" { | ||
| res += "#" + tag.Relation.String() | ||
| } | ||
| return res |
There was a problem hiding this comment.
user-id#? What is this doing? I forget id vs tag.Relation.String
There was a problem hiding this comment.
It's converting from the OpenFGA tag to a string. Lower down you'll see we had this logic duplicated for each resource type so I just captured it here.
ale8k
reviewed
Sep 2, 2024
| // [8] - A single ".", ignored | ||
| // [9] - Application offer name | ||
| // [10] - Relation specifier (i.e., #member) | ||
| // [2] - trailer (i.e. resource identifier) |
There was a problem hiding this comment.
We give example #member below, an identifier example would be good - like controller/model? I think that's this right?
There was a problem hiding this comment.
Not sure I follow, regarding usersets or the #<field> notation, we only have #member currently.
ale8k
approved these changes
Sep 2, 2024
| // [9] - Application offer name | ||
| // [10] - Relation specifier (i.e., #member) | ||
| // [2] - trailer (i.e. resource identifier) | ||
| // [3] - Relation specifier (i.e., #member) | ||
| // A complete matcher example would look like so with square-brackets denoting groups and paranthsis denoting index: |
There was a problem hiding this comment.
Parenthesis? Spelling error here aha
kian99
added a commit
to kian99/jimm
that referenced
this pull request
Sep 3, 2024
* tweak how tags are resolved - allow resolving tags without hitting the db if a UUID is specified * simplify matcher and tests * fix tests - Additionally cleaned up duplicated logic in ToJAASTag() * improve error message
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR does two things,
The first change was necessary when working on the Juju Terraform Provider because as part of testing that access has been removed, we have a test function that checks that tearing down the plan, a user has no access to a resource. But currently in JIMM if that resource doesn't exist the request will fail. But if the user is querying with a UUID there is no need to make a call to the database.
The second change was motivated by the idea that we were accepting a user input, decomposing that input into multiple components like "model name", "model owner", etc. Then recombining those fields. Instead, just pass the user input after extracting the relevant bits. This also simplifies our
jujuURIMatcherregex. The tests were also simplified.Partially addresses CSS-6347
Engineering checklist
Check only items that apply
Notes for code reviewers
I suggest reviewing this PR by each commit individually to see the changes more clearly.