fix: task returned from content_render missing task_id
#339
Conversation
R/deploy.R
Outdated
| @@ -580,7 +580,7 @@ poll_task <- function(task, wait = 1, callback = message) { | |||
| code <- -1 | |||
| first <- 0 | |||
| while (!finished) { | |||
| task_data <- con$task(task$get_task()$task_id, wait = wait, first = first) | |||
| task_data <- con$task(task$task$task_id, wait = wait, first = first) | |||
There was a problem hiding this comment.
You don't technically need this change right? get_task() just yields self$task. (Not opposed to ripping out the unnecessary getters, but that seems better done in a separate refactor PR where you did them all.)
There was a problem hiding this comment.
Correct, I don't technically need it; happy to revert if you think it's bad to change it here.
There was a problem hiding this comment.
I agree, for things like this which are easily greppable, no need to do it piecemeal.
R/content.R
Outdated
| @@ -1007,6 +1007,9 @@ content_render <- function(content, variant_key = NULL) { | |||
| target_variant <- get_variant(content, variant_key) | |||
| } | |||
| render_task <- target_variant$render() | |||
| # Tasks returned from variant render endpoint look different from those | |||
| # returned by deploy. | |||
| render_task$task_id <- render_task$id | |||
There was a problem hiding this comment.
I see this same logic in the Task initialize method: https://github.com/rstudio/connectapi/blob/main/R/deploy.R#L67-L69
Why doesn't that handle this for you already?
There was a problem hiding this comment.
Right, because of course VariantTask doesn't inherit from Task :/
I think the better fix would be to push this adapting into the VariantTask init method. Even better would be to look to fix the inheritance of all of these apparent (but not actual) Task subclasses, but that could be a followup--I'm not trying to derail a bugfix by tempting you into refactoring all the things.
There was a problem hiding this comment.
Yeah lol. But good point, I'll put it into VariantTask and rip out the other place that also does it.
I don't think ContentTask needs it because I don't think there are any content APIs that return this form.
Intent
Fix
poll_taskfor tasks returned bycontent_render.Fixes #338
Approach
The
POST variants/{variant_id}/renderendpoint returns an object that is shaped differently from the task returned by thedeploytask, with the task ID namedidand nottask_id. Thus, when creating aVariantTaskobject, we need to rename the parameter.I believe this was working in an initial draft of this functionality, but was changed in a subsequent PR that altered the return type for the function.
I added an integration test to verify
content_renderand the task it returns.Checklist
NEWS.md(referencing the connected issue if necessary)?devtools::document()?