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

feat: Upgrade to express 5.0.1 #9530

Merged
merged 23 commits into from
Mar 3, 2025
Merged

Conversation

pocketcolin
Copy link
Contributor

@pocketcolin pocketcolin commented Jan 10, 2025

Pull Request

Issue

Closes: #9353

Approach

Express 5 is not a huge change from 4, but it does introduce some breaking changes and deprecates body-parser (well technically that was replaced in a later version of 4, but I decided to clean it up in this PR anyway). Here are the main changes that impacted Parse Server in this upgrade and how I addressed them:

  1. Express moved route validation to a separate package called router. In Parse, path parameter validation is done using an Express class called Layer which is now in that router package so I added the new package and updated the import.
  2. req.body is now undefined if the body has not been parsed OR if there is no body. Previously, it returned an empty object. Because Parse attempts to access keys on body without verifying that it exists, I had to go through and update all unsafe reference attempts or places where body was passed to a function.
  3. body-parser has been replaced with native express functions. Updating this just meant swapping bodyParser with express and removing the package.
  4. urlencoded now defaults to false. I decided against removing the explicit setting of that property in ParseServer.js as a personal preference for more explicit declarations. It is no longer technically necessary though.
  5. Express 4 had a routing oddity that made double forward slashes between the router and route legal (discussed here: Double slash before Router (router//route) expressjs/express#4427). Definitely an anti-pattern, but unfortunately the Parse JS SDK actually uses it in the Hooks router. For the sake of backwards compatibility, I added a middleware that strips out the extra slash.
  6. In Express 4, extended query params were parsed automatically, but in Express 5 you have to explicitly tell it to do that by setting query parser to extended (setting defined here: https://expressjs.com/en/5x/api.html#app.set).
  7. Express 5 does not allow unnamed wildcard parameters. The only place Parse is using those is for custom static routes so I updated that and gave it a name. (https://expressjs.com/en/guide/migrating-5.html#path-syntax)

I also updated the package.json test scripts to use MongoDB 6.0.2 because 5.3.2 doesn't exist and you'll get an error if you try to run a test locally with that version.

Testing

The Express migration guide and release announcement was used to validate breaking changes:

Copy link

Thanks for opening this pull request!

@pocketcolin pocketcolin changed the title upgraded to express 5 with tests passing feat: upgraded to express 5 from 4 Jan 10, 2025
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat: upgraded to express 5 from 4 feat: Upgraded to express 5 from 4 Jan 10, 2025
@mtrezza
Copy link
Member

mtrezza commented Jan 10, 2025

Nice! It seems there are no breaking changes for developers in this PR, correct? I've started the CI.

@pocketcolin
Copy link
Contributor Author

@mtrezza I don't believe there are any! I also ran my app server's integration tests against the new build and they all passed.

@mtrezza mtrezza changed the title feat: Upgraded to express 5 from 4 feat: Upgrade to express 5.0.1 Jan 10, 2025
@mtrezza
Copy link
Member

mtrezza commented Jan 10, 2025

Do you know whether there are any effects on Parse Server that come with the upgrade? If there are no known effects, then we could merge this as a refactor (just like we do with any other dependency upgrade), which would not trigger a release.

@pocketcolin
Copy link
Contributor Author

That kinda depends on what you mean by effects. I haven't nailed down exactly why, but I haven't been able to update my app's version of Express to 5 without Parse crashing (stream is not readable) so this PR will allow users to upgrade to Express 5 in their apps and take advantage of all it has to offer. I'd suggest that because Express is such a core part of Parse and Express's upgrade from 4 to 5 was not a refactor, that upgrading Parse to use 5 is also not a refactor. That said, this change doesn't require anyone to make any updates so I could see going either way.

I'm seeing some errors in the tests that I was't seeing before so once those are fixed it I might find some more effects that would make the decision easier.

@mtrezza mtrezza added the state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message label Jan 12, 2025
@mtrezza
Copy link
Member

mtrezza commented Jan 12, 2025

I've marked this as a breaking change as a precaution; more analysis is needed before we can merge this and know whether it actually is a breaking change, or what its effects are.

  • Does it require that express 5.x is used to run Parse Server, like in the parse-server-example repo where express is used to mount Parse Server, or will it stay compatible with express 4.x?

@pocketcolin
Copy link
Contributor Author

That's fair, @mtrezza . To answer your one question though, I have tested this against a implementation running Express 4 without any issues so I believe this change will stay compatible with 4.x.

@pocketcolin
Copy link
Contributor Author

@mtrezza ok I've made a number of changes to get all of the tests passing and will update the PR description to reflect this. I still have not found anything that I would consider a breaking change, but these are the remaining things I found:

  • Express 4 had a routing oddity that made double forward slashes between the router and route legal. Definitely an anti-pattern, but unfortunately the Parse JS SDK actually uses it in the Hooks router. It would probably be a good idea to fix the Parse JS SDK so it didn't add a double slash, but for the sake of backwards compatibility, I added a middleware that strips out the extra slash. I'll also create a ticket in the JS SDK to clean this up.
  • In Express 4, extended query params were parsed automatically, but in Express 5 you have to explicitly tell it to do that by setting query parser to extended.
  • Express 5 does not allow unnamed wildcard parameters. The only place Parse is using those is for custom static routes so I updated that and gave it a name.

@pocketcolin pocketcolin requested a review from mtrezza January 13, 2025 21:15
@pocketcolin
Copy link
Contributor Author

@mtrezza all of the tests passed except for a series of 8 that failed for issues related to a bad cert.

  - Error: Hostname/IP does not match certificate's altnames: Host: cacerts.digicert.com. is not in the cert's altnames: DNS:ocsp.digicert.com

Any idea what that's about?

The code coverage in that report also seems fine to me but let me know what you think. I don't really think we need a test to cover that tiny conditional, but let me know if you feel differently!

@pocketcolin
Copy link
Contributor Author

@mtrezza anything else we need to do here (aside from getting tests to pass) before this is ready? Do you still feel like this is a breaking change and, if so, do you see this as being part of Parse 8 and not 7?

@pocketcolin
Copy link
Contributor Author

@mtrezza do you think we can get this into the initial launch of Parse 8? I just had to resolve some merge conflicts and I'd like to get it merged into the main branch so I don't have to keep doing that.

@pocketcolin
Copy link
Contributor Author

@mtrezza poke

@mtrezza
Copy link
Member

mtrezza commented Feb 7, 2025

You did a good job explaining the changes in this PR. Before we merge, I'd like to look more closely at any possible effects from upgrading to express 5. So we understand what that means for existing deployments that currently use express 4 and upgrade to Parse Server 8 which will then depend on express 5. We need to mention any breaking changes in the changelog, but rather than a blank "this is a breaking change using express 5, read the express docs to find out what you need to do", I'd like to be more specific.

  • Developers who use an express 4 setup and only upgrade the Parse Server dependency, does that require any code changes on their side, and if so, which? Can they stay on express 4, or do they need to upgrade their setup to express 5?

  • For developers using body-parser in their express 4 setup, can they still use it, or did that become incompatible with express 5? You mention that body-parser has been replaced by native express functions. For example is there any change required for developers who currently use:

    expressApp.use(bodyParser.json());
    expressApp.use(bodyParser.urlencoded({ extended: true }));
    
  • How did you make the change in 23b9cc7 - did you manually edit the lockfile version?

@mtrezza
Copy link
Member

mtrezza commented Mar 2, 2025

@pocketcolin Thank you again for this PR. Apologies that it took some time, so we have a few more merge conflicts. Could you please resolve them, so we can merge this for the Parse Server 8 release? This is the last PR that is holding back the release. Regarding your question, we don't plan any effort to release this also for PS7, since we only fix security issues in releases under LTS. Given the potential for being a breaking change (I think we weren't able to fully exclude that possibility), we would also not be able to merge this into PS7.

@parse-community/server Any concerns regarding this?

@mtrezza mtrezza added the block:major Needs to be resolved before next major release; remove label afterwards label Mar 2, 2025
@parse-github-assistant parse-github-assistant bot removed the block:major Needs to be resolved before next major release; remove label afterwards label Mar 2, 2025
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.

Just the merge conflicts

@mtrezza mtrezza mentioned this pull request Mar 2, 2025
23 tasks
@mtrezza
Copy link
Member

mtrezza commented Mar 2, 2025

@dplewis Do you have any thoughts/concerns on this?

@mtrezza
Copy link
Member

mtrezza commented Mar 3, 2025

BREAKING CHANGE: This upgrades the internally used Express framework from version 4 to 5, which may be a breaking change. If Parse Server is set up to be mounted on an Express application, we recommend to also use version 5 of the Express framework to avoid any compatibility issues. Note that even if there are no issues after upgrading, future releases of Parse Server may introduce issues if Parse Server internally relies on Express 5-specific features which are unsupported by the Express version on which it is mounted. See the Express migration guide and release announcement for more info.

@pocketcolin
Copy link
Contributor Author

@mtrezza sorry I missed your questions! Here are some answers:

  • No, you should not need to run Express 5 on your project in order to upgrade to Parse 8 running Express 5. I have tested that in my own projects and theoretically that's how all projects should work because of how node isolates packages (though from experience I know this isn't always true).
  • No change for devs using bodyParser for the same reason. If you were doing expressApp.use(bodyParser.json()) then you were also installing bodyParser as a package in your project. Removing it from Parse will have no impact on that.
  • I have no idea what happened with the lockfileVersion change. A git check flagged it as an error and after investigating I just switched it back. Clearly something I ran locally triggered the version to change, but I can't tell you what that was.

Thanks for resolving the conflicts and merging alpha into this for me! Let me know if there's anything else I can do.

@mtrezza
Copy link
Member

mtrezza commented Mar 3, 2025

@pocketcolin Thanks for providing more details and the PR. I think we can merge this as is now.

@mtrezza mtrezza merged commit e0480df into parse-community:alpha Mar 3, 2025
24 checks passed
parseplatformorg pushed a commit that referenced this pull request Mar 3, 2025
# [8.0.0-alpha.15](8.0.0-alpha.14...8.0.0-alpha.15) (2025-03-03)

### Features

* Upgrade to express 5.0.1 ([#9530](#9530)) ([e0480df](e0480df))

### BREAKING CHANGES

* This upgrades the internally used Express framework from version 4 to 5, which may be a breaking change. If Parse Server is set up to be mounted on an Express application, we recommend to also use version 5 of the Express framework to avoid any compatibility issues. Note that even if there are no issues after upgrading, future releases of Parse Server may introduce issues if Parse Server internally relies on Express 5-specific features which are unsupported by the Express version on which it is mounted. See the Express [migration guide](https://expressjs.com/en/guide/migrating-5.html) and [release announcement](https://expressjs.com/2024/10/15/v5-release.html#breaking-changes) for more info. ([e0480df](e0480df))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 8.0.0-alpha.15

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label Mar 3, 2025
parseplatformorg pushed a commit that referenced this pull request Mar 4, 2025
# [8.0.0](7.4.0...8.0.0) (2025-03-04)

### Bug Fixes

* LiveQueryServer crashes using cacheAdapter on disconnect from Redis 4 server ([#9616](#9616)) ([bbc6bd4](bbc6bd4))
* Push adapter not loading on some versions of Node 22 ([#9524](#9524)) ([ff7f671](ff7f671))
* Remove username from email verification and password reset process ([#8488](#8488)) ([d21dd97](d21dd97))
* Security upgrade node from 20.17.0-alpine3.20 to 20.18.2-alpine3.20 ([#9583](#9583)) ([8f85ae2](8f85ae2))

### Features

* Add dynamic master key by setting Parse Server option `masterKey` to a function ([#9582](#9582)) ([6f1d161](6f1d161))
* Add support for MongoDB `databaseOptions` keys `autoSelectFamily`, `autoSelectFamilyAttemptTimeout` ([#9579](#9579)) ([5966068](5966068))
* Add support for MongoDB `databaseOptions` keys `minPoolSize`, `connectTimeoutMS`, `socketTimeoutMS` ([#9522](#9522)) ([91618fe](91618fe))
* Add TypeScript support ([#9550](#9550)) ([59e46d0](59e46d0))
* Change default value of Parse Server option `encodeParseObjectInCloudFunction` to `true` ([#9527](#9527)) ([5c5ad69](5c5ad69))
* Deprecate `PublicAPIRouter` in favor of `PagesRouter` ([#9526](#9526)) ([7f66629](7f66629))
* Increase required minimum MongoDB versions to `6.0.19`, `7.0.16`, `8.0.4` ([#9531](#9531)) ([871e508](871e508))
* Increase required minimum Node versions to `18.20.4`, `20.18.0`, `22.12.0` ([#9521](#9521)) ([4e151cd](4e151cd))
* Increase required minimum versions to Postgres `15`, PostGIS `3.3` ([#9538](#9538)) ([89c9b54](89c9b54))
* Upgrade to express 5.0.1 ([#9530](#9530)) ([e0480df](e0480df))
* Upgrade to Parse JS SDK 6.0.0 ([#9624](#9624)) ([bf9db75](bf9db75))

### BREAKING CHANGES

* This upgrades the internally used Express framework from version 4 to 5, which may be a breaking change. If Parse Server is set up to be mounted on an Express application, we recommend to also use version 5 of the Express framework to avoid any compatibility issues. Note that even if there are no issues after upgrading, future releases of Parse Server may introduce issues if Parse Server internally relies on Express 5-specific features which are unsupported by the Express version on which it is mounted. See the Express [migration guide](https://expressjs.com/en/guide/migrating-5.html) and [release announcement](https://expressjs.com/2024/10/15/v5-release.html#breaking-changes) for more info. ([e0480df](e0480df))
* This upgrades to the Parse JS SDK 6.0.0. See the [change log](https://github.com/parse-community/Parse-SDK-JS/releases/tag/6.0.0) of the Parse JS SDK for breaking changes and more details. ([bf9db75](bf9db75))
* This removes the username from the email verification and password reset process to prevent storing personally identifiable information (PII) in server and infrastructure logs. Customized HTML pages or emails related to email verification and password reset may need to be adapted accordingly. See the new templates that come bundled with Parse Server and the [migration guide](https://github.com/parse-community/parse-server/blob/alpha/8.0.0.md) for more details. ([d21dd97](d21dd97))
* This releases increases the required minimum versions to Postgres `15`, PostGIS `3.3` and removes support for Postgres `13`, `14`, PostGIS `3.1`, `3.2`. ([89c9b54](89c9b54))
* The default value of Parse Server option `encodeParseObjectInCloudFunction` changes to `true`; the option has been deprecated and will be removed in a future version. ([5c5ad69](5c5ad69))
* This releases increases the required minimum MongoDB versions to `6.0.19`, `7.0.16`, `8.0.4` and removes support for MongoDB `4`, `5`. ([871e508](871e508))
* This releases increases the required minimum Node versions to 18.20.4, 20.18.0, 22.12.0 and removes unofficial support for Node 19. ([4e151cd](4e151cd))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message state:released-alpha Released as alpha version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade express from 4 to 5
4 participants