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

replace {httr} with {curl} #249

Merged
merged 17 commits into from
Feb 24, 2025
Merged

replace {httr} with {curl} #249

merged 17 commits into from
Feb 24, 2025

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Feb 23, 2025

fixes #229

  • replaces {httr} with {curl}
  • adds some integration tests covering HTTP-request code (that don't rely on Elasticsearch)

Notes for Reviewers

How I tested this

The existing integration tests cover all the Elasticsearch interactions.

To check that the retry logic works, I built the package from source and installed it.

cd r-pkg/
R CMD INSTALL .

Then, in R:

futile.logger::flog.threshold(futile.logger::DEBUG)

# request that should succeed
res <- uptasticsearch:::.request(
    verb = "GET"
    , url = "https://httpbin.org/status/429"
    , body = NULL
    , add_json_headers = FALSE
)

Saw logs like this:

DEBUG [2025-02-22 23:07:40] Request failed (status code 429): 'GET https://httpbin.org/status/429'
DEBUG [2025-02-22 23:07:40] Sleeping for 1.88 before retrying.
DEBUG [2025-02-22 23:07:42] Request failed (status code 429): 'GET https://httpbin.org/status/429'
DEBUG [2025-02-22 23:07:42] Sleeping for 2.62 before retrying.
DEBUG [2025-02-22 23:07:45] Request failed (status code 429): 'GET https://httpbin.org/status/429'

And a response object returned:

res$method
# [1] "GET"

res$status_code
# [1] 429

res$url
# [1] "https://httpbin.org/status/429"

@jameslamb jameslamb added the maintenance miscellaneous maintenance label Feb 23, 2025
Comment on lines -249 to -253
, "1" = .process_legacy_alias
, "2" = .process_legacy_alias
, "5" = .process_new_alias
, "6" = .process_new_alias
, .process_new_alias
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out we didn't need any of this stuff!!!

I realized through this exercise that the difference across Elasticsearch versions was like this:

  • we weren't passing the Accept header in this request to GET /_cat/aliases
  • on ES 1.x and 2.x, GET /_cat/aliases responded with fixed-width text by default
  • from ES 3.x onwards, it responded with JSON data by default
  • across ALL versions, it responds with JSON data if you pass Accept: application/json

So just passing that header, this now always returns JSON and no more need for these different processing functions 😁 😁 😁

Copy link
Collaborator

Choose a reason for hiding this comment

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

Amazing!

@jameslamb jameslamb changed the title WIP: replace {httr} with {curl} replace {httr} with {curl} Feb 24, 2025
@jameslamb jameslamb marked this pull request as ready for review February 24, 2025 04:05
@jameslamb jameslamb mentioned this pull request Feb 24, 2025
13 tasks
Copy link
Collaborator

@austin3dickey austin3dickey left a comment

Choose a reason for hiding this comment

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

Looks great!

Comment on lines -249 to -253
, "1" = .process_legacy_alias
, "2" = .process_legacy_alias
, "5" = .process_new_alias
, "6" = .process_new_alias
, .process_new_alias
Copy link
Collaborator

Choose a reason for hiding this comment

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

Amazing!

# set headers
#
# This can safely be hard-coded here because every payload this library
# posts and every response body it receives is JSON data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool!

response <- .request(
verb = "GET"
, url = "https://httpbin.org/status/502"
, body = NULL
Copy link
Collaborator

Choose a reason for hiding this comment

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

I forget, does one of the other integration tests use a body? If not maybe we should add one just for a lil more coverage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Many do use it implicitly... for example es_search() is going to issue a POST request with the query attached as a body.

But agree it'd be good to have an explicit one here, to cover that if() block. I'll add that before merging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just pushed 51ee836 with 2 more tests, explicitly covering the case where requests have a body.

@jameslamb
Copy link
Collaborator Author

Those new tests are passing, R CMD check looks happy... onwards, to CRAN!!

Thanks as always @austin3dickey

@jameslamb jameslamb merged commit 18df38e into uptake:main Feb 24, 2025
15 checks passed
@jameslamb jameslamb deleted the switch-to-curl branch February 25, 2025 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance miscellaneous maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop {httr} dependency
2 participants