RFD-322: users list (+ filter by group)#2423
Conversation
| let opctx = OpContext::for_external_api(&rqctx).await?; | ||
| let users = nexus | ||
| .silo_users_list_current(&opctx, &pagparams) | ||
| .silo_users_list_current_by_id(&opctx, &pagparams) |
There was a problem hiding this comment.
I could make them both use the same method like @zephraph did here if the group filter query param is purely additive.
https://github.com/oxidecomputer/omicron/pull/2420/files#diff-7429b402f97c8a901b88c3599e8505de4afae78092b5f27e4542bc5818125f17R5794-R5798
david-crespo
force-pushed
the
group-list-users
branch
from
79239d7 to
3730b35
Compare
| }] | ||
| async fn user_list_v1( | ||
| rqctx: RequestContext<Arc<ServerContext>>, | ||
| query_params: Query<PaginatedById<params::OptionalGroupSelector>>, |
There was a problem hiding this comment.
Is the group selector actually optional?
There was a problem hiding this comment.
The way I pictured it working was: if you don't specify a group, you get all users. If you specify a group you get the users in that group.
| self.db_datastore | ||
| .silo_users_list(opctx, &authz_silo_user_list, pagparams) | ||
| .await | ||
| } |
There was a problem hiding this comment.
I'm splitting the logic here, but really these could be two Nexus functions. In fact, I'll make that change. I'm remembering the wisdom of Sandi Metz: move logic forks as high in the call stack as you can.
There was a problem hiding this comment.
Not sure about the quote but it does make sense to me to break this down.
| let scan_params = ScanById::from_query(&query)?; | ||
| // TODO: a valid UUID gets parsed here and will 404 if it doesn't exist | ||
| // (as expected) but a non-UUID string just gets let through as None | ||
| // (i.e., ignored) instead of 400ing |
There was a problem hiding this comment.
Hmmm, I'll have to dig into the underlying mechanism here. What we could do is add validation in ScanById::from_query that returns an error if the UUID is invalid. The flow here is a bit different though so I want to make sure I'm understanding it better.
There was a problem hiding this comment.
That’s a good point. We could also do it semi-manually here. I was thinking it might need some serde attribute thing, but that would prevent the struct from being used in a place where we don’t want it to error out.
There was a problem hiding this comment.
Interestingly, this fixed it. The nested serde flatten was making it more lenient somehow.
5402185#diff-aee01ef600c8eb547421397e5d89c41a67dd6e6ca6033676cac47b7909e1e344
There was a problem hiding this comment.
That is interesting. Once we're done with the V1 migration a next big step is wrapping up the selectors in a macro or something and potentially simplifying them. It won't result in an API change but I definitely feel like there's some improvement to be had there.
david-crespo
force-pushed
the
group-list-users
branch
from
91ce79b to
5402185
Compare
# Conflicts: # openapi/nexus.json
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "title": "ScanById", | ||
| "title": "ScanById_for_Null", |
There was a problem hiding this comment.
This seems fine. I don't think this title actually ends up being used by anything.
david-crespo
changed the title
david-crespo
changed the title
| use db::schema::silo_group_membership as sgm; | ||
| use db::schema::silo_user as su; |
There was a problem hiding this comment.
Minor thing but could we change sgm to group and su to user? I think that might make it a bit easier to read the query below.
david-crespo
force-pushed
the
group-list-users
branch
from
14a779d to
9967c0f
Compare
2 participants
Needed to make
PaginatedByIdtake a genericSelectorparam so I could give it the optional group selector.