feat: include vanity urls in get_content #399

toph-allen · 2025-04-29T16:21:52Z

Intent

Include vanity URLs in get_content() data frame.

Fixes #398

Approach

Include vanity URLs in get_content() results.
Update get_content() documentation to detail all returned fields.

I considered a slightly more complex approach, bubbling up include to the top level and allowing users to customize the returned data, but the performance penalty for including all the fields seems negligible, so I don't think it's worth the time and complexity, and decided to keep things as simple as possible.

N.B. Integration tests haven't finished yet, this might require a test change…

Checklist

Does this change update NEWS.md (referencing the connected issue if necessary)?
Does this change need documentation? Have you run devtools::document()?

nealrichardson · 2025-04-29T16:25:27Z

R/get.R

@@ -203,6 +220,7 @@ get_content <- function(
  guid = NULL,
  owner_guid = NULL,
  name = NULL,
+  include = c("tags", "owner", "vanity_url"),


I know vanity_url was added relatively recently to the API here. How should this behave for older versions of Connect where it's not available here?

Oh, I didn't realize that it was new! I think this PR isn't ready for prime time — I'll mark it as a draft and ping you when it's ready, after I've done some experimentation.

Looks like it was added in 2024.06.0: https://docs.posit.co/connect/news/#posit-connect-2024.06.0-new

So not new new, but well within the support window.

Thanks. Added version-keyed behavior in the latest push, plus tests to make sure the correct requests are sent to different versions (the endpoint returns a 400 if an invalid parameter is supplied)

toph-allen · 2025-04-29T22:55:04Z

tests/testthat/test-get.R

+test_that("get_content only requests vanity URLs for Connect 2024.06.0 and up", {
+  with_mock_dir("2024.05.0", {
+    client <- Connect$new(server = "http://connect.example", api_key = "not-a-key")
+    client$version


version is fetched lazily, so calling it here just ensure that it's fetched from the correct mock dir before we expect_GET within without_internet().

How about this, to memorialize that comment and also be explicit:

Suggested change

client$version

# Call $version here because it is fetched lazily, and we need it populated

# before we do `without_internet()`

expect_equal(client$version, "2024.05.0")

If you agree, you'll want to do the same for the other two tests

Yeah — if it was worth a comment for the reviewer, probably worth a comment for future editors of the code. :)

toph-allen · 2025-04-30T14:59:23Z

@nealrichardson Now has Connect-version-dependent behavior.

nealrichardson

This looks like a good direction!

nealrichardson · 2025-04-30T17:36:33Z

tests/testthat/test-get.R

+test_that("get_content only requests vanity URLs for Connect 2024.06.0 and up", {
+  with_mock_dir("2024.05.0", {
+    client <- Connect$new(server = "http://connect.example", api_key = "not-a-key")
+    client$version


How about this, to memorialize that comment and also be explicit:

Suggested change

client$version

# Call $version here because it is fetched lazily, and we need it populated

# before we do `without_internet()`

expect_equal(client$version, "2024.05.0")

If you agree, you'll want to do the same for the other two tests

nealrichardson · 2025-04-30T17:37:38Z

tests/testthat/test-get.R

+  without_internet({
+    expect_GET(
+      get_content(client),
+      "http://connect.example/__api__/v1/content?include=tags%2Cowner%2Cvanity_url"


I'm guessing that you'll need to update the other mocks for GET v1/content/, at least the mock file paths to expect this query string.

None of the other mocks seem to have broken, actually — no failed tests, just an angry linter.

That's odd, do we not have mock tests for get_content()? I would expect something to fail, unless we're only testing on the old API and were always including tags and owner before?

I see what you mean.

Looking at this more in more depth: the default mock path is named "2024.08.0" but actually runs with an empty server version (which is a legitimate Connect configuration, but a poor name for that directory.)

I've been mulling over in the back of my head how to address this — the sheer number of files in the mock directory makes it kind of difficult to grapple with in any way that, like, changes the hashes of many requests at the same time.

I think in the short term, renaming the default mock path to "unversioned" or "null_version" would make it more obvious what's going on. I don't know if that addresses the underlying problem or not, though — I feel like there is an underlying problem with the organization of mocks (that's worth writing up in an issue / tackling separately).

One idea is remove the default mock path (https://github.com/posit-dev/connectapi/blob/main/tests/testthat/setup.R#L36-L38) and always use with_mock_dir() explicitly pointing at a path. And/or yes, having a dir that is for the case where the API suppresses the server version could be useful for something I guess.

To my original question: the default for include in connect$content() is tags,owner, so on the no-version-exposed API, the request is the same as it was before, so we load the same mock we were before.

One idea is remove the default mock path (main/tests/testthat/setup.R#L36-L38) and always use with_mock_dir() explicitly pointing at a path. And/or yes, having a dir that is for the case where the API suppresses the server version could be useful for something I guess.

Yeah, I think this might be the case, at least going forward. Even code that doesn't explicitly check a version of Connect is tested against mocks captured from a specific version, and good practice might be to just use that mock dir.

And yep, your answer to your original question is correct.

R/get.R

…it-dev/connectapi into toph/get-content-vanity-urls

nealrichardson

Looks good. I think there are some followups to create:

Sounds like you have something in mind about the mock directories, per the discussion in the comment.
It would be good to take a pass through the other functions that do stuff with vanity URLs and see if they can be simplified at least on new Connect. get_vanity_url, for example, may already have the vanity URL and not need to make an additional request.

toph-allen · 2025-05-01T17:38:09Z

@nealrichardson Thanks, I'll file those

toph-allen added 2 commits April 29, 2025 12:01

include vanity urls in get_content

254d58b

update NEWS

f3ce762

toph-allen requested review from jonkeane and nealrichardson April 29, 2025 16:22

nealrichardson reviewed Apr 29, 2025

View reviewed changes

toph-allen marked this pull request as draft April 29, 2025 16:28

toph-allen added 4 commits April 29, 2025 12:29

fix accidental extra argument

a5e5f05

fix extra arg part 2

3069671

handle connect versions, test

3010936

update NEWS

1ed1408

toph-allen requested a review from nealrichardson April 29, 2025 22:54

toph-allen commented Apr 29, 2025

View reviewed changes

toph-allen marked this pull request as ready for review April 30, 2025 14:58

Merge branch 'main' into toph/get-content-vanity-urls

880fe88

nealrichardson reviewed Apr 30, 2025

View reviewed changes

toph-allen changed the title ~~fix: include vanity urls in get_content~~ feat: include vanity urls in get_content Apr 30, 2025

toph-allen added 2 commits April 30, 2025 14:30

add comment, appease linter

6e2e4d4

Merge branch 'toph/get-content-vanity-urls' of https://github.com/pos…

fb1984e

…it-dev/connectapi into toph/get-content-vanity-urls

toph-allen requested a review from nealrichardson April 30, 2025 18:31

nealrichardson approved these changes Apr 30, 2025

View reviewed changes

Merge branch 'main' into toph/get-content-vanity-urls

23d0d1a

This was referenced May 1, 2025

chore: reorganize mocks #401

Open

chore: check functions that get content vanity URLs to see if they can be simplified #402

Open

toph-allen merged commit e8c8075 into main May 1, 2025
19 checks passed

toph-allen deleted the toph/get-content-vanity-urls branch May 1, 2025 19:30

-    client$version
+    # Call $version here because it is fetched lazily, and we need it populated
+    # before we do `without_internet()`
+    expect_equal(client$version, "2024.05.0")

feat: include vanity urls in get_content #399

feat: include vanity urls in get_content #399

Uh oh!

Conversation

toph-allen commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Intent

Approach

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toph-allen commented Apr 30, 2025

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toph-allen Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nealrichardson Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

toph-allen commented May 1, 2025

Uh oh!

Uh oh!

Uh oh!

toph-allen commented Apr 29, 2025 •

edited

Loading

toph-allen Apr 30, 2025 •

edited

Loading

nealrichardson Apr 30, 2025 •

edited

Loading