Change default setting of query parser from extended to simple

[czaarek99]

Implemented the change discussed in #3361

[wesleytodd]

Thanks @czaarek99! Change looks good, can you just squash these into one commit?

@dougwilson Can we look at getting these query parser changes merged before the next 5.x beta (this and #3259)? I think we should focus on getting as many of these breaking changes merged so we can start educating users on the big changes they should be ready for.

[dougwilson]

Shouldn't be deprecate it in 4.x first before suddenly changing the behavior in 5? All the other changes have gone through a deprecation cycle so I think this should as well.

[wesleytodd]

In a perfect world we would introduce a deprecation in 5 then make the change in 6. If we had a faster release cycle I would agree, but I think we need to improve the default security settings sooner than later.

I am just worried that if we start printing a deprecation warning in a 4.x branch because it will just cause a ton of churn like the new Buffer or the React.propTypes deprecation warnings did. But if you think this is worth it than I wouldn't argue strongly against it.

[dougwilson]

I think a change that is as subtle as this needs a depreciation cycle prior to outright change. If something is being changed / removed in 5 and it is possible to deprecate in 4 it should be deprecated first in 4 and then the change in the 5 branch would be the change plus removing deprecation.

[dougwilson]

The body-parser module did this and yes there was some churn at the beginning but no one thinks about it any longer. Besides the fact that deprecations are warnings and can simply be turned off if someone wants.

[czaarek99]

@dougwilson When would printing this deprecation message be suitable? On startup?

[dougwilson]

It should probably act similar to how the express.urlencoded works for deprecation on the extended option.

[czaarek99]

@dougwilson But isn't that different though? In the case of express.urlencoded you have to call a function on which you'd print the deprecation. In this case we'd have to print the warning if they don't specifically call app.set('query parser', 'extended'), but when would that be?

Shouldn't we print the error if we detect an incoming extended object and we haven't received an app.set('query parser', 'extended') before that?

[mattsre]

@wesleytodd This closes #3361. Given your timeline for releasing 5.0 end-of-month is it still worth adding a deprecation message in 4.x?

[dougwilson]

but when would that be?

At the time the query middleware is created.

[gireeshpunathil]

I would like a reassessment of the proposed changes in this PR - specifically, on the deprecation in 4.x part - is this still valid, given we are in the brink of 5?

@czaarek99 - are you in position to progress on this PR, collaborating with the community reviewers? this is tagged as an important item that needs to be included in 5.

If yes, let me know if you need any help
If not, no worries, I am willing to take this up and move forward

[dougwilson]

I would like a reassessment of the proposed changes in this PR - specifically, on the deprecation in 4.x part - is this still valid, given we are in the brink of 5?

Since 4.x is still going to exist for quite some time once 5 is released it is still valid. If it doesn't get deprecated in 4 before releasing 5, then it would need to be deprecated in 5 and then released in 6. This is trying to follow the Node.js model, which we have been doing so far for anything that can possibly be marked as deprecated.

If it is not possible to add a deprecation to 4, then we can reevaluate deprecation, though I don't think that is what you are saying here.

[gireeshpunathil]

@dougwilson - so (if we want to deprecate in 4 before 5 is out), we are looking for an imminent 4.x release, right? is it (deprecation) going to be a minor, or a patch?

[dougwilson]

Yes, there is already an imminent 4 release that will be before this weekend's 5 release: expressjs/discussions#100 (comment)

[gireeshpunathil]

ping @dougwilson - can you merge this and #4208 to move forward, as discussed? thanks!

[dougwilson]

Hi @gireeshpunathil sorry, the #4208 needs to be merged first. I just commented on there in that there still seems to be an unresolved issue I uncovered when I went to do the merging the other night, if you can take a look.

[dougwilson]

Visibility for @gireeshpunathil : in the TC meeting regarding this, it was pointed out that express.urlencoded needs the same change so they are in line with each other. I have been working on that one myself so both changes can be landed into the 5.0 branch.

The main blocker here remains the landing of the deprecation PR.

[czaarek99]

I would like a reassessment of the proposed changes in this PR - specifically, on the deprecation in 4.x part - is this still valid, given we are in the brink of 5?

@czaarek99 - are you in position to progress on this PR, collaborating with the community reviewers? this is tagged as an important item that needs to be included in 5.

If yes, let me know if you need any help
If not, no worries, I am willing to take this up and move forward

I opened this pull request about two years ago just to learn a little bit about express and how it works. You can do whatever you guys think is right but at this point I don't even remember why this PR was opened and why it was so hard getting it merged. You guys can go ahead and figure it out without me, no worries 😃

[gireeshpunathil]

@czaarek99 - I understand your frustration. I have added this to the upcoming TC meeting to reflect and figure out what went wrong / what could have been done better.

[czaarek99]

@gireeshpunathil Did we get anywhere? 💭

[dougwilson]

Hi @czaarek99 ; I we have a PR to add a deprecation, and I had my teams test it out last week while I was away while I was awaiting the feedback just recently providing around docs for the 4.18 release -- I am writing up a response in that PR about the issue it has and my reasoning as to why I don't think we should actually end up deprecating it in the 4.x release, which would mean your PR here would just land directly on 5.0 prior to 4.18 completing, as it would no longer be dependent.

Ultimately, I which we did actually try to do the deprecation back in 2018 when the discussion first occurred instead of now, but what is past is past. We are working hard to go forward with where we are now.

[czaarek99]

Hi @czaarek99 ; I we have a PR to add a deprecation, and I had my teams test it out last week while I was away while I was awaiting the feedback just recently providing around docs for the 4.18 release -- I am writing up a response in that PR about the issue it has and my reasoning as to why I don't think we should actually end up deprecating it in the 4.x release, which would mean your PR here would just land directly on 5.0 prior to 4.18 completing, as it would no longer be dependent.

Ultimately, I which we did actually try to do the deprecation back in 2018 when the discussion first occurred instead of now, but what is past is past. We are working hard to go forward with where we are now.

Makes sense. Thanks for the update :)

[czaarek99]

Since this is going into 5.x wouldn't it be preferable to merge it into that branch ASAP and get it out in the alpha npm releases to be able to watch out for potential bugs this might lead to?

[dougwilson]

Yes, this is landing in the very next 5.0 release we are making.