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

[Epic] New HTTP Adaptor & Utils #457

Merged
merged 143 commits into from
Jan 24, 2024
Merged

[Epic] New HTTP Adaptor & Utils #457

merged 143 commits into from
Jan 24, 2024

Conversation

josephjclark
Copy link
Collaborator

@josephjclark josephjclark commented Jan 11, 2024

Epic PR for all the http and common work.

Closes #316 #320 #304

Changes

  • New http helpers in common
  • New http adaptor
  • deprecated old common.http
  • update template to showcase new pattern
  • check and maybe improve docs
  • remove axios, superagent and request across the repo
  • Add linting for undeclared variables (and fix a whole bunch of stuff)

Overview

This PR has the broad aim of removing axios, superagent and request in favour of a single HTTP client - undici.

But we're not quite doing that yet as it's too painful.

As a starting point, we have added a new set of http helper functions (functions not operations!) into common, which you'll find in common/util/http. These are wrappers around undici. We intend that all new and existing adaptors migrate onto these utils.

The headline feature for this release is HTTP 6.0, which is the existing HTTP adaptor "rebased" onto the common helpers, with axios removed. This comes with loads of breaking changes, so there's a thorough migration guide in the http changelog. HTTP should now be a very lightweight adaptor which just wraps the common helpers.

The template has been changed to show usage of the new common helpers.

Finally, the docs output has been improved (internal links with upper case characters fixed; magic metadata removed; better documentation for http options added through a typedef) and linting has been strengthened (resulting in many small but critical patches on existing adaptors).

@josephjclark

This comment was marked as resolved.

@josephjclark
Copy link
Collaborator Author

Added a cheeky fix for #304

@josephjclark
Copy link
Collaborator Author

@mtuchi caught an issue where parseXML was broken because expandReferences isn't imported.

This wasn't picked up in unit testing or linting, which is a bit scary.

I don't want to to write a parseXML unit test now, so I have added a linting rule for undeclared variables. This triggers a rash of patch fixes to various adaptors.

@josephjclark
Copy link
Collaborator Author

josephjclark commented Jan 19, 2024

Running some tests with Mtuchi I think I really need to do slightly better error handling immediately.

The error message we get back right now does not include any message from the server - just the status code and status message.

If the server returns an error message, we need to include it in the error object

Edit: push a commit to return the response body and headers on the error object, and to log the error body when errors occur. This improves the feedback we're giving.

packages/http/CHANGELOG.md Outdated Show resolved Hide resolved
packages/http/CHANGELOG.md Outdated Show resolved Hide resolved
@stuartc
Copy link
Member

stuartc commented Jan 23, 2024

If the server returns an error message, we need to include it in the error object

This is great! A lot of people will be very happy. I can't remember why (if any reason) we didn't do this before? I know there has like always been security considerations around this, like someone logging headers that have API keys in them.

Perhaps it's a non-issue now - but perhaps worth asking @taylordowns2000 why we previously cut error responses from HTTP requests to almost nothing.

@josephjclark
Copy link
Collaborator Author

@stuartc The error comments are more related to this branch than to main. I think the error reporting used to be mostly fine - the big problem was that the whole response object was logged, leaking all kinds of stuff and spamming the output.

The new adaptor builds an error object and returns hand-picked keys, which is a problem if we return the wrong keys.

I do want to enable access to the raw request, just in case, but there's a separate issue for that. I think we should do it soonish. Part of the problem is it's hard to test this stuff, and when you have a test implementation it's tempting to lean into it a bit too hard.

@stuartc stuartc assigned stuartc and unassigned stuartc Jan 24, 2024
@stuartc stuartc self-requested a review January 24, 2024 07:30
@josephjclark josephjclark merged commit 56ae0dd into main Jan 24, 2024
1 check passed
mtuchi added a commit that referenced this pull request Jan 24, 2024
* wip add request util

* wip: using undici client

add tests

* wip: add mock agent

* wip: more tests

* wip: test options

* update undici

* add changeset

* fix typo

* improve errorMap

* fix undici

* remove method in assertOK

* improve signature

* improve assertOK

add tests for baseUrl option and errorMap

* throw an error if response code match errorMap

* remove duplicate property value

* test if body is string and add default body timeout

* wip: improvements in util

* remove util.js

* restructure util files

* add recursive option

* common: restructure build output

* remove comments

* update isValidHttpUrl

minor improvements

* rename a test

* wip: parseAs

* wip: update mockClient func

* wip: refactor test

* improve tests

* better url parsing

* remove comment

* update response body

* update content type check

* remove comments

* remove headers

* common: adjust import

* common: tweak request docs

* wip: refactor

* wip: refactor test

* wip: post tests

* wip: skip failed tests

* wip: tests

* common: updated unit tests

* common: fix test

* add request helper function

* wip: clean up tests

* add support for successcode in errorMap

* fix broken test

* remove undici skip test

* remove cookies handling

* remove unused packages

* improve aut parsing

* wip: followRedirect

* wip: follow redirect

* wip: form-data

* http: update formdata tests

* http: remove rediect test

* improve logging

* clean up unused packages and add http-status-codes

* clean up deadcode

* improve form-data test

* add changeset

* remove common changeset

* improvements

* add duration

* add method and url to response body

* add support for tls

* improve jsdoc

* update changelog

* add requestBodyType function

This function assert that body type and convert a plan json to a string

* remove JSON.stringify

* add unit test for requestBodyType

* add helpers for http server

* update dependencies

* remove http helpers

* add intergration test

* add responseUrl function

This function will return history url if found or request url

* fix test

* add koa and koa-sslify

* add certificates for testing

* update tests

* move server into integration file

* add server logic

* update https port

it turns out in ubuntu any port < 1024 needs root permission

* use nodes https module

remove koa implementation

* http: move request helper into utils

* common: typo in docs

* http: fix headers and start updating tests

* package lock

* test updates

* common: rename http helper response touse statusCode and statusMessage. Also remove the response url.

* http: update tests

* http: update tests

* common: restore url to the response object

A version of it anyway

* http: refactor response log

* common: update http error message

* http: update to use new  error handling

* http: tidy ups

* http: docs

* http,common:docs

* http: undocument maxRedirects

Not willing to commit to it tbh

* http: light refactor

* common: another light refactor

* lock node version in ci

* common: restore default error map

* http: fix form processing

* http: remove file extension from import

* common: deprecate old http functions

Shoud print a warning in dev mode, but not in prod (platform or lightning)

* http: expose a general request operation

* common: update release notes

* bump versions

* lockfile

* Update auto-generated documentation.

* docs: improve markdown templates

* tools: update docs

* Update http docs

* http: update request example

* remove build artefacts

* http,common: move some http stuff into common utils

* Update template

* template: update to use new helpers

* template: remove unused code

* readme

* Update template

Template should be locked at 1.0.0 and never published

* common: don't print output if you're given a bad response

* common: better url building

* http: remove custom tags from docs

* common: tweak URL parsing rules

* common: better url reporting in errors

* common: fix url parsing

whoops

* common: don't use URL.parse

* add linting for undeclared variables

Fixes issues in azure-storage, common, http, magpi, mailchimp, mongodb, resourcemap and twilio

* bump versions

* common: better handling of error responses in http

* http: update changelog

* http: changelog fixes

---------

Co-authored-by: Emmanuel Evance <mtuchidev@gmail.com>
@josephjclark josephjclark deleted the epic/http branch March 5, 2024 15:36
mtuchi added a commit that referenced this pull request Mar 19, 2024
* wip add request util

* wip: using undici client

add tests

* wip: add mock agent

* wip: more tests

* wip: test options

* update undici

* add changeset

* fix typo

* improve errorMap

* fix undici

* remove method in assertOK

* improve signature

* improve assertOK

add tests for baseUrl option and errorMap

* throw an error if response code match errorMap

* remove duplicate property value

* test if body is string and add default body timeout

* wip: improvements in util

* remove util.js

* restructure util files

* add recursive option

* common: restructure build output

* remove comments

* update isValidHttpUrl

minor improvements

* rename a test

* wip: parseAs

* wip: update mockClient func

* wip: refactor test

* improve tests

* better url parsing

* remove comment

* update response body

* update content type check

* remove comments

* remove headers

* common: adjust import

* common: tweak request docs

* wip: refactor

* wip: refactor test

* wip: post tests

* wip: skip failed tests

* wip: tests

* common: updated unit tests

* common: fix test

* add request helper function

* wip: clean up tests

* add support for successcode in errorMap

* fix broken test

* remove undici skip test

* remove cookies handling

* remove unused packages

* improve aut parsing

* wip: followRedirect

* wip: follow redirect

* wip: form-data

* http: update formdata tests

* http: remove rediect test

* improve logging

* clean up unused packages and add http-status-codes

* clean up deadcode

* improve form-data test

* add changeset

* remove common changeset

* improvements

* add duration

* add method and url to response body

* add support for tls

* improve jsdoc

* update changelog

* add requestBodyType function

This function assert that body type and convert a plan json to a string

* remove JSON.stringify

* add unit test for requestBodyType

* add helpers for http server

* update dependencies

* remove http helpers

* add intergration test

* add responseUrl function

This function will return history url if found or request url

* fix test

* add koa and koa-sslify

* add certificates for testing

* update tests

* move server into integration file

* add server logic

* update https port

it turns out in ubuntu any port < 1024 needs root permission

* use nodes https module

remove koa implementation

* http: move request helper into utils

* common: typo in docs

* http: fix headers and start updating tests

* package lock

* test updates

* common: rename http helper response touse statusCode and statusMessage. Also remove the response url.

* http: update tests

* http: update tests

* common: restore url to the response object

A version of it anyway

* http: refactor response log

* common: update http error message

* http: update to use new  error handling

* http: tidy ups

* http: docs

* http,common:docs

* http: undocument maxRedirects

Not willing to commit to it tbh

* http: light refactor

* common: another light refactor

* lock node version in ci

* common: restore default error map

* http: fix form processing

* http: remove file extension from import

* common: deprecate old http functions

Shoud print a warning in dev mode, but not in prod (platform or lightning)

* http: expose a general request operation

* common: update release notes

* bump versions

* lockfile

* Update auto-generated documentation.

* docs: improve markdown templates

* tools: update docs

* Update http docs

* http: update request example

* remove build artefacts

* http,common: move some http stuff into common utils

* Update template

* template: update to use new helpers

* template: remove unused code

* readme

* Update template

Template should be locked at 1.0.0 and never published

* common: don't print output if you're given a bad response

* common: better url building

* http: remove custom tags from docs

* common: tweak URL parsing rules

* common: better url reporting in errors

* common: fix url parsing

whoops

* common: don't use URL.parse

* add linting for undeclared variables

Fixes issues in azure-storage, common, http, magpi, mailchimp, mongodb, resourcemap and twilio

* bump versions

* common: better handling of error responses in http

* http: update changelog

* http: changelog fixes

---------

Co-authored-by: Emmanuel Evance <mtuchidev@gmail.com>
mtuchi added a commit that referenced this pull request Mar 19, 2024
* wip add request util

* wip: using undici client

add tests

* wip: add mock agent

* wip: more tests

* wip: test options

* update undici

* add changeset

* fix typo

* improve errorMap

* fix undici

* remove method in assertOK

* improve signature

* improve assertOK

add tests for baseUrl option and errorMap

* throw an error if response code match errorMap

* remove duplicate property value

* test if body is string and add default body timeout

* wip: improvements in util

* remove util.js

* restructure util files

* add recursive option

* common: restructure build output

* remove comments

* update isValidHttpUrl

minor improvements

* rename a test

* wip: parseAs

* wip: update mockClient func

* wip: refactor test

* improve tests

* better url parsing

* remove comment

* update response body

* update content type check

* remove comments

* remove headers

* common: adjust import

* common: tweak request docs

* wip: refactor

* wip: refactor test

* wip: post tests

* wip: skip failed tests

* wip: tests

* common: updated unit tests

* common: fix test

* add request helper function

* wip: clean up tests

* add support for successcode in errorMap

* fix broken test

* remove undici skip test

* remove cookies handling

* remove unused packages

* improve aut parsing

* wip: followRedirect

* wip: follow redirect

* wip: form-data

* http: update formdata tests

* http: remove rediect test

* improve logging

* clean up unused packages and add http-status-codes

* clean up deadcode

* improve form-data test

* add changeset

* remove common changeset

* improvements

* add duration

* add method and url to response body

* add support for tls

* improve jsdoc

* update changelog

* add requestBodyType function

This function assert that body type and convert a plan json to a string

* remove JSON.stringify

* add unit test for requestBodyType

* add helpers for http server

* update dependencies

* remove http helpers

* add intergration test

* add responseUrl function

This function will return history url if found or request url

* fix test

* add koa and koa-sslify

* add certificates for testing

* update tests

* move server into integration file

* add server logic

* update https port

it turns out in ubuntu any port < 1024 needs root permission

* use nodes https module

remove koa implementation

* http: move request helper into utils

* common: typo in docs

* http: fix headers and start updating tests

* package lock

* test updates

* common: rename http helper response touse statusCode and statusMessage. Also remove the response url.

* http: update tests

* http: update tests

* common: restore url to the response object

A version of it anyway

* http: refactor response log

* common: update http error message

* http: update to use new  error handling

* http: tidy ups

* http: docs

* http,common:docs

* http: undocument maxRedirects

Not willing to commit to it tbh

* http: light refactor

* common: another light refactor

* lock node version in ci

* common: restore default error map

* http: fix form processing

* http: remove file extension from import

* common: deprecate old http functions

Shoud print a warning in dev mode, but not in prod (platform or lightning)

* http: expose a general request operation

* common: update release notes

* bump versions

* lockfile

* Update auto-generated documentation.

* docs: improve markdown templates

* tools: update docs

* Update http docs

* http: update request example

* remove build artefacts

* http,common: move some http stuff into common utils

* Update template

* template: update to use new helpers

* template: remove unused code

* readme

* Update template

Template should be locked at 1.0.0 and never published

* common: don't print output if you're given a bad response

* common: better url building

* http: remove custom tags from docs

* common: tweak URL parsing rules

* common: better url reporting in errors

* common: fix url parsing

whoops

* common: don't use URL.parse

* add linting for undeclared variables

Fixes issues in azure-storage, common, http, magpi, mailchimp, mongodb, resourcemap and twilio

* bump versions

* common: better handling of error responses in http

* http: update changelog

* http: changelog fixes

---------

Co-authored-by: Emmanuel Evance <mtuchidev@gmail.com>
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.

http: rewrite to use new helper functions
4 participants