Skip to content

Accept integers or characters for hit id column #512

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.

Sign up for GitHub

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

Merged
jonkeane merged 3 commits into from
Mar 5, 2026
Merged

Accept integers or characters for hit id column #512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4677b76
Accept integers or characters for hit id
jonkeane Mar 4, 2026
96eb4f1
🗞️
jonkeane Mar 4, 2026
211f6ef
Update R/parse.R
jonkeane Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# connectapi (development version)

- `get_usage()` now returns the id column as a character to match other parts of the API.

Comment on lines +3 to +4
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

Changing the id column type from integer to character is a breaking change for existing users of get_usage() (introduced in 0.8.0) who may be relying on integer comparisons or integer arithmetic on the id column. The other recent releases (0.10.0, 0.11.0) follow the pattern of calling out such changes under a ## Breaking changes subsection. Consider adding a ## Breaking changes subsection to the development version entry noting this type change.

Suggested change
- `get_usage()` now returns the id column as a character to match other parts of the API.
## Breaking changes
- `get_usage()` now returns the `id` column as a character to match other parts of the API. This may break code that relies on `id` being an integer (for example, integer comparisons or arithmetic on `id`).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@jonkeane jonkeane Mar 5, 2026

Choose a reason for hiding this comment

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

@karawoo thoughts? My inclination is to leave it where it is, we already marked it as breaking on the connect server, and for most (all?) people this change in types will be entirely inconsequential. But I can add the header if you think it's worth it

Copy link
Collaborator

@nealrichardson nealrichardson Mar 5, 2026

Choose a reason for hiding this comment

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

I think of this more as a fix than a breaking change because you only noticed this because the ptype stuff threw errors right? And there is no real use for this field otherwise.

Copy link
Collaborator

@karawoo karawoo Mar 5, 2026

Choose a reason for hiding this comment

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

we already discussed separately but yeah, I think it's fine where it is.

# connectapi 0.11.0

- `get_usage()` now allows for filtering by content GUID with the `content_guid`
Expand Down
2 changes: 1 addition & 1 deletion R/get.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ get_usage_static <- function(
#' @return A list of usage records. Each record is a list with all elements
#' as character strings unless otherwise specified.
#'
#' * `id`: An integer identifier for the hit.
#' * `id`: A string identifier for the hit.
#' * `user_guid`: The user GUID if the visitor is logged-in, `NULL` for
#' anonymous hits.
#' * `content_guid`: The GUID of the visited content.
Expand Down
8 changes: 8 additions & 0 deletions R/parse.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ ensure_column <- function(data, default, name) {
col <- bit64::as.integer64(col)
}

if (is.character(default) && (is.integer(col) || is.double(col))) {
if (is.double(col)) {
col <- format(col, scientific = FALSE, trim = TRUE)
} else {
col <- as.character(col)
}
}

if (inherits(default, "list") && !inherits(col, "list")) {
col <- list(col)
}
Expand Down
2 changes: 1 addition & 1 deletion R/ptype.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ connectapi_ptypes <- list(
"data_version" = NA_integer_
),
usage = tibble::tibble(
"id" = NA_integer_,
"id" = NA_character_,
Copy link
Collaborator

@nealrichardson nealrichardson Mar 5, 2026

Choose a reason for hiding this comment

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

This is the id... of the hit? What if we just dropped it? I can't imagine it has any value.

Copy link
Collaborator Author

@jonkeane jonkeane Mar 5, 2026

Choose a reason for hiding this comment

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

Yeah, that would make sense, except that it will have value 🔜 (in the work I'm doing to unify hits + shiny usage we need something to join on, and this is the thing to join on). We can drop it now if we want, though we will need to pull it back when I get through my unification / cleanup work.

"user_guid" = NA_character_,
"content_guid" = NA_character_,
"timestamp" = NA_datetime_,
Expand Down
2 changes: 1 addition & 1 deletion man/get_usage.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions tests/testthat/test-get.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,8 @@ with_mock_dir("2025.04.0", {

expect_length(usage, 5)

# Check first element
# Check first element (raw list, before conversion to data.frame).
# The id is numeric in the JSON, so it stays numeric in the raw list.
expect_equal(
usage[[1]],
list(
Expand All @@ -408,7 +409,7 @@ with_mock_dir("2025.04.0", {
expect_equal(
usage_df,
data.frame(
id = c(8966707L, 8966708L, 8967206L, 8967210L, 8966214L),
id = c("8966707", "8966708", "8967206", "8967210", "8966214"),
user_guid = c(NA, NA, NA, NA, "fecbd383"),
content_guid = c(
"475618c9",
Expand Down
43 changes: 43 additions & 0 deletions tests/testthat/test-parse.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,3 +344,46 @@ test_that("parse_connectapi handles mixed null/non-null integer timestamps", {
expect_type(result$end_time, "double")
expect_identical(result$end_time, c(NA_real_, 1732556770))
})

test_that("ensure_column coerces integer to character when ptype expects character", {
# Old Connect returns integer IDs
int_data <- tibble::tibble(id = c(100L, 200L, 300L))
result <- ensure_column(int_data, NA_character_, "id")
expect_type(result$id, "character")
expect_identical(result$id, c("100", "200", "300"))

# New Connect returns character IDs

char_data <- tibble::tibble(id = c("100", "200", "300"))
result <- ensure_column(char_data, NA_character_, "id")
expect_type(result$id, "character")
expect_identical(result$id, c("100", "200", "300"))
})

test_that("parse_connectapi_typed produces character id for usage with integer input", {
# Simulates old Connect returning integer IDs
int_hits <- list(
list(id = 123L, user_guid = "abc", content_guid = "def",
timestamp = "2025-04-30T12:00:00Z", data = list(path = "/test")),
list(id = 456L, user_guid = "ghi", content_guid = "jkl",
timestamp = "2025-04-30T13:00:00Z", data = list(path = "/other"))
)

result <- parse_connectapi_typed(int_hits, connectapi_ptypes$usage)
expect_type(result$id, "character")
expect_identical(result$id, c("123", "456"))
})

test_that("parse_connectapi_typed produces character id for usage with character input", {
# Simulates new Connect returning character IDs
char_hits <- list(
list(id = "123", user_guid = "abc", content_guid = "def",
timestamp = "2025-04-30T12:00:00Z", data = list(path = "/test")),
list(id = "456", user_guid = "ghi", content_guid = "jkl",
timestamp = "2025-04-30T13:00:00Z", data = list(path = "/other"))
)

result <- parse_connectapi_typed(char_hits, connectapi_ptypes$usage)
expect_type(result$id, "character")
expect_identical(result$id, c("123", "456"))
})
Loading