-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: get_group_content()
lets you get content that groups can access [draft, tests forthcoming]
#337
base: main
Are you sure you want to change the base?
Conversation
This is still a draft, as I haven't finished writing tests yet. I'd still appreciate feedback on the direction. |
Both |
@schloerke How much do you think I should hesitate to change |
Marking this as ready for review. There are some areas that might be over-engineered! The function supports passing in bare Originally I thought we needed the name to filter the permissions list, but I mistakenly thought that the In the future (or possibly now?) I'll update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a higher level: do we need to have this dual-input setup where one can pass either guids or a data.frame? What happens if the data.frame passed has data that is contradictory a guid and name in the same row but those don't match what's on the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added contextual comments for reviewers.
R/connect.R
Outdated
#' @description Get content to which a group has access | ||
#' @param guid The group GUID. | ||
group_content = function(guid) { | ||
path <- v1_url("experimental", "groups", guid, "content") | ||
self$GET(path) | ||
}, | ||
|
||
#' @description Get the details for a group | ||
#' @param guid The group GUID. | ||
group_details = function(guid) { | ||
path <- v1_url("groups", guid) | ||
self$GET(path) | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two new endpoints to support new group functionality. Makes me wonder if I should add a function to call the group details endpoint, to get the details from just the GUID. Probably not high priority for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is group_details()
called anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was previously, when I was looking up the group name for GUID-only invocations. But it's not now, so I've removed it.
R/groups.R
Outdated
#' @param limit The number of groups to retrieve before paging stops. | ||
#' | ||
#' `limit` will be ignored is `prefix` is not `NULL`. | ||
#' To limit results when `prefix` is not `NULL`, change `page_size`. | ||
#' | ||
#' @return | ||
#' A tibble with the following columns: | ||
#' | ||
#' * `guid`: The unique identifier of the group | ||
#' * `name`: The group name | ||
#' * `owner_guid`: The group owner's unique identifier. When using LDAP or | ||
#' Proxied authentication with group provisioning enabled this property | ||
#' will always be null. | ||
#' | ||
#' @details | ||
#' Please see https://docs.posit.co/connect/api/#get-/v1/groups for more information. | ||
#' | ||
#' @examples | ||
#' \dontrun{ | ||
#' library(connectapi) | ||
#' client <- connect() | ||
#' | ||
#' # get all groups | ||
#' get_groups(client, limit = Inf) | ||
#' } | ||
#' | ||
#' @family groups functions | ||
#' @export | ||
get_groups <- function(src, page_size = 500, prefix = NULL, limit = Inf) { | ||
validate_R6_class(src, "Connect") | ||
|
||
# The `v1/groups` endpoint always returns the first page when `prefix` is | ||
# specified, so the page_offset function, which increments until it hits an | ||
# empty page, fails. | ||
if (!is.null(prefix)) { | ||
response <- src$groups(page_size = page_size, prefix = prefix) | ||
res <- response$results | ||
} else { | ||
res <- page_offset(src, src$groups(page_size = page_size, prefix = NULL), limit = limit) | ||
} | ||
|
||
out <- parse_connectapi_typed(res, connectapi_ptypes$groups) | ||
|
||
return(out) | ||
} | ||
|
||
#' Get users within a specific group | ||
#' | ||
#' @param src The source object | ||
#' @param guid A group GUID identifier | ||
#' | ||
#' @return | ||
#' A tibble with the following columns: | ||
#' | ||
#' * `email`: The user's email | ||
#' * `username`: The user's username | ||
#' * `first_name`: The user's first name | ||
#' * `last_name`: The user's last name | ||
#' * `user_role`: The user's role. It may have a value of administrator, | ||
#' publisher or viewer. | ||
#' * `created_time`: The timestamp (in RFC3339 format) when the user | ||
#' was created in the Posit Connect server | ||
#' * `updated_time`: The timestamp (in RFC3339 format) when the user | ||
#' was last updated in the Posit Connect server | ||
#' * `active_time`: The timestamp (in RFC3339 format) when the user | ||
#' was last active on the Posit Connect server | ||
#' * `confirmed`: When false, the created user must confirm their | ||
#' account through an email. This feature is unique to password | ||
#' authentication. | ||
#' * `locked`: Whether or not the user is locked | ||
#' * `guid`: The user's GUID, or unique identifier, in UUID RFC4122 format | ||
#' | ||
#' @details | ||
#' Please see https://docs.posit.co/connect/api/#get-/v1/groups/-group_guid-/members | ||
#' for more information. | ||
#' | ||
#' @examples | ||
#' \dontrun{ | ||
#' library(connectapi) | ||
#' client <- connect() | ||
#' | ||
#' # get the first 20 groups | ||
#' groups <- get_groups(client) | ||
#' | ||
#' group_guid <- groups$guid[1] | ||
#' | ||
#' get_group_members(client, guid = group_guid) | ||
#' } | ||
#' | ||
#' @family groups functions | ||
#' @export | ||
get_group_members <- function(src, guid) { | ||
validate_R6_class(src, "Connect") | ||
|
||
res <- src$group_members(guid) | ||
|
||
out <- parse_connectapi(res$results) | ||
|
||
return(out) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first part of the file (get_groups()
and get_group_members()
) were just moved verbatim from get.R
. I think it's probably more helpful to organize the code consistently by subject domain, and to move away from the paradigm of having most of the "get this type of object" functions in get.R
.
R/groups.R
Outdated
#' Get content access permissions for a group or groups | ||
#' | ||
#' @param src The source object | ||
#' @param groups Either a data frame of groups, or a character vector of group guids | ||
#' | ||
#' @return | ||
#' A tibble with the following columns: | ||
|
||
#' * `group_guid`: The group's GUID | ||
#' * `group_name`: The group's name | ||
#' * `content_guid`: The content item's GUID | ||
#' * `content_name`: The content item's name | ||
#' * `content_title`: The content item's title | ||
#' * `access_type`: The access type of the content item ("all", "logged_in", or "acl") | ||
#' * `role`: The access type that members of the group have to the | ||
#' content item, "publisher" or "viewer". | ||
#' | ||
#' @examples | ||
#' \dontrun{ | ||
#' library(connectapi) | ||
#' client <- connect() | ||
#' | ||
#' # Get a data frame of groups | ||
#' groups <- get_groups(client) | ||
#' | ||
#' # Get permissions for a single group by passing in the corresponding row. | ||
#' get_group_content(client, groups[1, ]) | ||
#' dplyr::filter(groups, name = "research_scientists") %>% | ||
#' get_group_content(client, groups = .) | ||
#' | ||
#' # Get permissions for all groups by passing in the entire groups data frame. | ||
#' get_group_content(client, groups) | ||
#' | ||
#' # You can also pass in a guid or guids as a character vector. | ||
#' get_group_content(client, groups$guid[1]) | ||
#' } | ||
#' | ||
#' @family groups functions | ||
#' @export | ||
#' @importFrom rlang .data | ||
get_group_content <- function(src, groups) { | ||
validate_R6_class(src, "Connect") | ||
if (inherits(groups, "data.frame")) { | ||
validate_df_ptype(groups, tibble::tibble( | ||
guid = NA_character_, | ||
name = NA_character_ | ||
)) | ||
} else if (inherits(groups, "character")) { | ||
# If a character vector, we assume we are receiving group guids, and call | ||
# the endpoint to fetch the group name. | ||
groups <- purrr::map_dfr(groups, src$group_details) | ||
} else { | ||
stop("`groups` must be a data frame or character vector.") | ||
} | ||
|
||
purrr::pmap_dfr( | ||
dplyr::select(groups, .data$guid, .data$name), | ||
get_group_content_impl, | ||
src = src | ||
) | ||
} | ||
|
||
#' @importFrom rlang .data | ||
get_group_content_impl <- function(src, guid, name) { | ||
validate_R6_class(src, "Connect") | ||
|
||
res <- src$group_content(guid) | ||
parsed <- parse_connectapi_typed(res, connectapi_ptypes$group_content) | ||
|
||
dplyr::transmute(parsed, | ||
group_guid = guid, | ||
group_name = name, | ||
.data$content_guid, | ||
.data$content_name, | ||
.data$content_title, | ||
.data$access_type, | ||
role = purrr::map_chr( | ||
.data$permissions, | ||
extract_role, | ||
principal_guid = guid | ||
) | ||
) | ||
} | ||
|
||
# Given the list of permissions for a content item, extract the role for the | ||
# provided principal_guid | ||
extract_role <- function(permissions, principal_guid) { | ||
matched <- purrr::keep( | ||
permissions, | ||
~ .x[["principal_guid"]] == principal_guid | ||
) | ||
if (length(matched) == 1) { | ||
return(matched[[1]][["principal_role"]]) | ||
} else { | ||
stop("Unexpected permissions structure.") | ||
} | ||
stop(glue::glue("Could not find permissions for \"{principal_guid}\"")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New functions that comprise the implementations of the get_group_content()
, factored out slightly for clarity of code and testability.
Notes for reviewers here, which should probably become code comments:
- The exported function,
get_group_content()
, handles input validation and normalization (to allow the use of both GUIDs and data frames), and callsget_group_content_impl()
across all input groups. get_group_content_impl()
gets the content for a single group from the guid and name. It isn't exported because it is the strictly less flexible core ofget_group_content()
.extract_role()
is factored out for unit tests. Probably also useful elsewhere.
), | ||
group_content = tibble::tibble( | ||
content_guid = NA_character_, | ||
content_name = NA_character_, | ||
content_title = NA_character_, | ||
access_type = NA_character_, | ||
permissions = NA_list_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For use with parse_connectapi_typed()
R/ptype.R
Outdated
|
||
# Validates an input data frame against a required schema ptype. | ||
# 1. is a data frame or similar object; | ||
# 2. contains all the names from the required; | ||
# 3. that all matching names have the correct ptype. | ||
validate_df_ptype <- function(input, required) { | ||
if (!inherits(input, "data.frame")) { | ||
stop("Input must be a data frame.") | ||
} | ||
if (!all(names(input) %in% required)) { | ||
missing <- setdiff(names(required), names(input)) | ||
if (length(missing) > 0) { | ||
stop(glue::glue("Missing required columns: {paste0(missing, collapse = ', ')}")) | ||
} | ||
} | ||
|
||
for (col in names(required)) { | ||
tryCatch( | ||
vctrs::vec_ptype_common(input[[col]], required[[col]]), | ||
error = function(e) { | ||
stop(glue::glue( | ||
"Column `{col}` has type `{vctrs::vec_ptype_abbr(input[[col]])}`; ", | ||
"needs `{vctrs::vec_ptype_abbr(required[[col]])}:`\n", | ||
conditionMessage(e) | ||
)) | ||
} | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to allow us to easily validate data frames against a schema in functions, and to give decent user-interpretable error messages.
tests/testthat/_snaps/groups.md
Outdated
Warning: | ||
Use of .data in tidyselect expressions was deprecated in tidyselect 1.2.0. | ||
i Please use `"guid"` instead of `.data$guid` | ||
Warning: | ||
Use of .data in tidyselect expressions was deprecated in tidyselect 1.2.0. | ||
i Please use `"name"` instead of `.data$name` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These warnings don't appear when I use the function from the command line. As far as I can tell, there are no other good options here:
- Just using
guid
with no qualifications gives lint errors - Using
"guid"
fixes lint errors but changes the names of the resulting data frame to"guid"
with quotation marks(!)
I missed this from earlier. My fault. https://github.com/search?q=get_group_members+language%3AR+&type=code shows there isn't any usage outside of Posit. The shape of the function isn't really changing. The second argument would still be But it seems fair to just try this new style moving forward and when the day comes, we can catch up |
@jonkeane I think it's really nice to be able to pass in a data frame; it means that you can directly use the output of As to your second question: If the user passes in an arbitrary data frame with If they pass in a data frame that has real There are a few ways we could avoid this, but they all involve some level of compromise:
Another option, which I like less, would be to just implement this in a way that operates on a single group and does not include a P.S. Another upcoming story (I'm starting on it now) where rubber ducking about how it should feel led to passing in the returned data frame or a row from it rather than individual parameters: terminating jobs |
@schloerke I imagine that most of the usage of |
If we only accepted GUIDs, you could still go from
I actually had missed in my first reading of the code that we are using the input data.frame to construct the output for group names. That's fishy — it opens the rest of the code up to having to deal with edge cases that we shouldn't (need to).
nods yeah, those edge cases sound annoying to deal with. It might mostly work in most cases, but guarding against them is nontrivial (one example of this is in the PR itself: there's a non-trivial amount of guarding code that we have here, and if we push this forward we would likely need even more)
To be consistent: the approach of accepting data.frames also has compromises in the amount of guarding code we need to write + the edge cases we need to address when supporting that. Additionally, there is a documentation + user burden to having an argument that accepts two types and has slightly different behavior based on that. Sometimes it's worth it to do that anyway, but I don't think it is in this case. Would it be possible to implement this PR as just accepting GUIDs? There are open questions about possible optimizations (getting group names) or alternative inputs (data.frames). But getting the simple case of GUIDs in a good state and merging that first would do us well here. |
Ok, I can do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much more minimal, which is a great 💯 . A few comments about reducing some of the nesting of functions
permissions_df <- purrr::map_dfr( | ||
parsed$permissions, | ||
~ purrr::keep( | ||
.x, | ||
~ .x[["principal_guid"]] == guid | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes through the permissions (list of data.frames) column and returns a dataframe that is the matching rows from that list, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
permissions
is a list of lists… of lists, hence the nestedness and the use of purrr::keep
.
- At top level, each item is a content item i.e. a row of the data frame.
- The second level (i.e. within each each row), each item is a
principal
— potentially multiple per content item. - In the third level, each item is a property of that principal permissions record (
principal_name
,principal_role
,principal_guid
,principal_type
, etc.).
The API returns all the principals for the content item, so we have to filter for the correct principal for each content item.
R/groups.R
Outdated
} | ||
|
||
#' @importFrom rlang .data | ||
get_group_content_impl <- function(src, guid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this separate level of nested functions here still? I think it made more sense when there were a few more branches, but it feels unnecessary here.
I also wonder if we can reduce the number of (practical) loops we're going through. We call this in a purrr::map..
up above, and then there is at least one purrr::map...
below which includes two anonymous functions which also practically go through each element checking for equality. These might all be necessary, but when I was trying to simplify this found the levels of nesting to be not-totally-obvious what was doing what where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure. I found the distinction at this level to be helpful when thinking about the code — thinking about parsing each group's content into a data frame and then mapping that across map_dfr
was helpful. But it would be possible to put this in an anonymous function call.
It isn't immediately obvious to me how to reduce the level of nestedness.
- This level is necessary, because we receive multiple GUIDs, and each one requires some level of processing.
- The inner processing is fairly nested because only the top level of the incoming JSON is parsed into a data frame, and the API returns a fairly nested structure for
permissions
(see below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This level is necessary, because we receive multiple GUIDs, and each one requires some level of processing.
Maybe we could make the name of this function make this clearer? If it were something like get_one_group_content
that makes it a bit clearer that it's getting content for a single guid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I like that change.
R/connect.R
Outdated
#' @description Get content to which a group has access | ||
#' @param guid The group GUID. | ||
group_content = function(guid) { | ||
path <- v1_url("experimental", "groups", guid, "content") | ||
self$GET(path) | ||
}, | ||
|
||
#' @description Get the details for a group | ||
#' @param guid The group GUID. | ||
group_details = function(guid) { | ||
path <- v1_url("groups", guid) | ||
self$GET(path) | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is group_details()
called anywhere?
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
Intent
Add a function to support getting content that a group has access to.
Fixes #334
Approach
The new function,
get_group_content()
, requires asrc
argument andgroups
, which can either be a data frame withguid
andname
columns or a character vector of GUIDs.This is slightly different from similar functions like
get_group_members(client, group_guid)
, which requires only the guid. Reasons for the difference:group_guid
is marginally simpler for the user for getting one group's content (just select the row, don't need to pull the column), and significantly simpler for getting multiple groups (just pass in a data frame with multiple rows). Landed upon this approach & reasoning rubber-ducking with @schloerke earlier last week.group_name
in the returned data frame with less overhead. The downside of this is, to keep our return shape consistent, we need to look up thegroup_name
for each group if passed a bare character vector of GUIDs.Other notes
groups.R
file.Checklist
NEWS.md
(referencing the connected issue if necessary)?devtools::document()
?