Skip to content

fix: Replace outdated HTTP request module #8282

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

Closed
wants to merge 5 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Nov 3, 2022

New Pull Request Checklist

Issue Description

Currently, Parse Server uses a custom HTTP implementation to perform request for hooks, tests, as well as Parse.Cloud.httpRequest

Related issue: #7589
Closes: #7589

Approach

Replaces existing module with axios.

TODOs before merging

  • Add tests
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Nov 3, 2022

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@dblythy
Copy link
Member Author

dblythy commented Nov 3, 2022

This also un-depreciates Parse.Cloud.httpRequest, although now the parameters need to be according to axios.

@dblythy
Copy link
Member Author

dblythy commented Nov 3, 2022

Changes work for 99% of tests, however for some (3) tests the new request method fails. I've added request.legacy to circumvent this.

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 94.11% // Head: 94.07% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (17387b6) compared to base (81304be).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 17387b6 differs from pull request most recent head 1e185c7. Consider uploading reports for the commit 1e185c7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8282      +/-   ##
==========================================
- Coverage   94.11%   94.07%   -0.04%     
==========================================
  Files         182      182              
  Lines       13775    13811      +36     
==========================================
+ Hits        12964    12993      +29     
- Misses        811      818       +7     
Impacted Files Coverage Δ
src/cloud-code/Parse.Cloud.js 99.19% <100.00%> (-0.02%) ⬇️
src/cloud-code/httpRequest.js 90.19% <100.00%> (-8.25%) ⬇️
src/Routers/ClassesRouter.js 95.91% <0.00%> (-1.03%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.78% <0.00%> (+0.07%) ⬆️
src/Adapters/Storage/Mongo/MongoTransform.js 88.57% <0.00%> (+0.15%) ⬆️
src/Adapters/Files/GridFSBucketAdapter.js 94.16% <0.00%> (+0.72%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mtrezza mtrezza linked an issue Nov 3, 2022 that may be closed by this pull request
3 tasks
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

It would be better if we could have 1 PR per issue, because they are different in nature; one is an internal refactor and unrelated to Parse Server 6, the other is a breaking change. Could you

@mtrezza mtrezza changed the title fix: Replace existing HTTP request module fix: Replace outdated HTTP request module Nov 3, 2022
@dblythy
Copy link
Member Author

dblythy commented Nov 3, 2022

Here I am suggesting keeping Parse.Cloud.httpRequest as an alias to axios.

I'm not quiet sure how they are seperate issues - in order to remove the old method, we need to introduce a new one so that the tests can pass (as the tests extensively use the http module)

@mtrezza
Copy link
Member

mtrezza commented Nov 3, 2022

Issue 1 is to remove the Parse.Cloud.httpRequest method as described in #8271. The method is deprecated, so it should not be available anymore in Cloud Code for developers to use. That has nothing to do with the fact that the http request depedency is outdated.

Issue 2 is to replace http request with axios as described in #7589, which is only an internal refactor, once issue 1 is done, because it's not exposed through Parse.Cloud.httpRequest anymore.

You could imagine that issue 2 doesn't exist, that we won't switch to axios. Issue 1 would only remove the method Parse.Cloud.httpRequest.

@dblythy
Copy link
Member Author

dblythy commented Nov 4, 2022

If that is the case and we are not exposing it externally - is there any real purpose in replacing the HTTP module? Adding an extra dependency with the only production use of hooks, seems a bit extraneous. We could continue to use the HTTP module for tests (and perhaps refactor / update some of the depreciated methods) but remove Parse.Cloud.httpRequest

@mtrezza
Copy link
Member

mtrezza commented Nov 4, 2022

If that is the case and we are not exposing it externally - is there any real purpose in replacing the HTTP module?

I thought so because you wrote:

Why should we continue to support a legacy HTTP module, which hasn't been updated in 3 years, where there are more popular solutions that constantly update their security issues and vulnerabilities? The http module also uses deprecated methods, such as querystring and parse.

And I remember we have had some issues posted by users that the Parse.Cloud.httpRequest doesn't work for certain requests. So if we use the request lib anywhere internally, we should probably replace it? I think node 18 comes with the native fetch built in, which may make an external lib unnecessary, but we're still supporting node 14.

@mtrezza
Copy link
Member

mtrezza commented Nov 4, 2022

I suggest to first remove the external method in a separate PR, and then modify this PR to replace the http lib. That may be easier than the other way around.

Do you think it's a lot of work to replace the request dependency with axios internally?

@dblythy
Copy link
Member Author

dblythy commented Nov 4, 2022

Ideally pretty much every test that uses request should be refactored

@dblythy
Copy link
Member Author

dblythy commented Nov 4, 2022

Closing as Node 18 will support native fetch, we will replace the internal http request module for tests with Parse Server 7.

@dblythy dblythy closed this Nov 4, 2022
@dblythy dblythy deleted the add-axios branch November 4, 2022 01:26
@mtrezza
Copy link
Member

mtrezza commented Nov 4, 2022

More details in #7589 (comment)

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.

Replace outdated http request dependency with Node 18's fetch
2 participants