Skip to content
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

fix: swap_vanity_urls() works predictably with unset vanity URLs #361

Merged
merged 9 commits into from
Jan 27, 2025

Conversation

toph-allen
Copy link
Collaborator

@toph-allen toph-allen commented Jan 10, 2025

Intent

Update swap_vanity_url() with numerous fixes and changes. Changes include:

  • When swap_vanity_url() was called on a piece of content without a vanity URL, the content item set to receive that would instead receive a randomized placeholder URL. Now the function works as intended when one content item has no vanity URL: the other content item receives an unset vanity URL!
  • Better error handling. Previously, errors were not handled gracefully, so if the caller did not have permission to modify the second content item, the first content item was left with its randomized placeholder URL. Now, errors are called out correctly and any changes made are rolled back.
  • Names! It is now called swap_vanity_urls() because you're swapping two vanity URLs between two content items. The arguments are now called content_a and content_b because the old argument names, from and to, don't make sense when vanity URLs move from and to both items. The old function name remains but is deprecated, and just shims argument and return object names to the new function.

Fixes #360

Approach

Rewrote the function's internal workflow. Pseudocode:

  1. Get both content item's current vanity URLs.
  2. Delete each content item's vanity URLs, each inside a tryCatch statement. If content_a fails, stop and err. If content_b fails, restore content_a's original vanity URL and err.
  3. Assign each content item the other's URL. We now know these operations will succeed, because delete and set have the same permissions.

Perhaps a better approach would be to call set_vanity_url(content_a, vanity_b, force = TRUE) in 2., as this doesn't risk the (admittedly very unlikely) case that another piece of content snipes the URL before it's reassigned. But this actually introduces more complexity:

  • We'd have to do conditional logic to know if we needed to set or delete in this step.
  • Because force = TRUE also modifies content_b's vanity URL, if we lacked permission to modify content_b, that operation would fail — but only if content_b initially had a vanity URL (if its vanity was unset, we'd just be unsetting vanity_a's URL). So the workflow for rollbacks would be much more complex, and harder to reason about / test / maintain.

Checklist

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

- delete_vanity_url() returns an error when it fails
- swap_vanity_urls() returns an error if it does not have permission to modify either of the content items’ vanity urls. if content_b fails, content_a’s url is rolled back.
@@ -464,14 +464,14 @@ delete_vanity_url <- function(content) {
error_if_less_than(con$version, "1.8.6")
guid <- content$get_content()$guid

con$DELETE(v1_url("content", guid, "vanity"))
con$DELETE(v1_url("content", guid, "vanity"), parser = "parsed")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that it returns an error if it receives an HTTP error status.

@toph-allen toph-allen requested a review from jonkeane January 24, 2025 22:07
@toph-allen toph-allen changed the title fix: swap_vanity_urls() works predictably with unset vanity URLs [DRAFT] fix: swap_vanity_urls() works predictably with unset vanity URLs Jan 24, 2025
@toph-allen toph-allen marked this pull request as ready for review January 24, 2025 22:08
now that the function works properly with unset vanities, the test condition was incorrect
@jonkeane jonkeane requested a review from tdstein January 27, 2025 20:40
Comment on lines +534 to +540
tryCatch(
delete_vanity_url(content_b),
error = function(e) {
set_vanity_url(content_a, vanity_a)
stop("Unable to modify the vanity URL for content_b: ", e$message, call. = FALSE)
}
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these transactions aren't atomic at the database level, there is no way to ensure the swap happens successfully. Nevertheless, these types of fallbacks are nice to have.

If logging is established in this project, I would add a log info statement describing each step as it happens. That way the user knows A was reset to its original state.

Comment on lines +547 to +548
vanity_a <- get_vanity_url(content_a)
vanity_b <- get_vanity_url(content_b)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is a good idea. The server does mutate the submitted vanity url in some cases (i.e., leading '/').

Copy link

@tdstein tdstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logical ✅
Test coverage ✅
Documentation ✅

Looks good to me!

@toph-allen toph-allen merged commit 002ed73 into main Jan 27, 2025
19 checks passed
@toph-allen toph-allen deleted the toph/fix-swap-vanity-urls-none branch January 27, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

swap_vanity_url(content1, content2) assigns junk URLs if one content item has no vanity set
2 participants