query: deprecate extended query parser

[gireeshpunathil]

Refs: #3621

• I thought about checking the parser type before throwing deprecation, but then found that it wasn't necessary, as we are hard-coding it to extended
• the query part in var deprecate = require('depd')('query'); is my assumption about how it should be, let me know if that is not the case
• feel free to propose changes to the message.

thanks!

[dougwilson]

I am expecting the following app to generate the deprecation warning with these changes, but I'm not seeing it. I believe I should be running the copy of your code, but just wanted to give it as a test case here if you wanted to confirm:

var express = require('express') // or './index' for testing
var app = express()

app.get('/', (req, res) => res.send('hi!'))

app.listen(3000)

[gireeshpunathil]

@dougwilson - yes, I can confirm that:

#cat 4208.js

var express = require('express') // or './index' for testing
var app = express()

app.get('/', (req, res) => res.send('hi!'))

app.listen(3000)

#node 4208.js

express deprecated default extended query parser deprecated, set explicitly to override query.js:4:5
^C

And on the other hand, if the user explicitly sets the parser, the warning is suppressed:

--- a.js	2020-03-10 09:03:02.000000000 +0530
+++ b.js	2020-03-10 09:02:39.000000000 +0530
@@ -1,5 +1,6 @@
 var express = require('express') // or './index' for testing
 var app = express()
+app.set('query parser', 'extended')
 
 app.get('/', (req, res) => res.send('hi!'))

[gireeshpunathil]

looks like const in strict mode broke CI. Pushed a change. @dougwilson - are you good now with the changes?

[dougwilson]

Thank you @gireeshpunathil ! I also got my local working correctly now to try out a couple things. I found the following example file is not generating a deprecation, when I would expect it to do so due to app2 using the deprecated API:

var express = require('express')
var app1 = express()
var app2 = express()

app1.set('query parser', false)
app1.get('/', (req, res) => res.send('hi!'))
app1.use('/api', app2)

app2.get('/users', (req, res) => res.json([]))

app1.listen(3000)

The above is an example of a basic sub app being mounted in an app. It seems that the order of declaration of the two (or more depending on the set up of users) apps will dictate if another app in the same process detects if the deprecated API is being used.

[gireeshpunathil]

@dougwilson - just pushed a change to address your scenario, please have a look!

[dougwilson]

Nice! I need to figure out what is wrong with my local environment tomorrow, because even though it says it pulled your updates, the code I posted above still prints no deprecation warnings when I run it. I need to figure out why my local is not picking up the fix.

[gireeshpunathil]

ping @dougwilson - this and #3621 - any blockers here? pls do the needful!

[dougwilson]

Hi @gireeshpunathil I have tried a few more times today to see if your latest comment resolved the issue above, but it does not seem to. I am definitely using your latest code, yet when I run the code I posted in #4208 (comment) there is still no deprecation message printed as expected. Are you 100% sure that is printing a deprecation message?

[gireeshpunathil]

@dougwilson - the code in that comment has explicit call to

app1.set('query parser', false)

right? as per the design you suggested, it is not supposed to print warning?

[dougwilson]

There are two apps, one which is not using a deprecated API (app1), but the second app (app2) is using the deprecated API, and should cause a deprecation to be emitted. You can see that app2 never sets the query parser, so it is using the default of extended, and this is supposed to print a deprecation warning when the user did not set the query parser on their app and instead is relying on the default behavior.

[gireeshpunathil]

(everyday is an opportunity to learn something new! I was under the impression that you were struggling to test the patch due to setup issues, did not realize you were waiting for me to fix anything. I am sure with close collaboration over time, we can understand each other)

so here is my perceived design:

no explicit setting of query parser: throw warning, one per app
explicit setting of query parser: don't throw warning, for the app that over-rode it.

is this how it should be according to you?

[gireeshpunathil]

pushed a change to that effect, PTAL!

[gireeshpunathil]

tests have passed; however continuous-integration/appveyor/branch seems to be waiting for something that I cannot figure out (no link, the preview text is partly hidden). Anything actionable from me here?
/cc @dougwilson

[dougwilson]

tests have passed; however continuous-integration/appveyor/branch seems to be waiting for something that I cannot figure out (no link, the preview text is partly hidden).

Yea, something changed on the AppVeyor side. I spent a couple hours investigating this on Sunday and got the issue resolved (you'll see it no longer pending on this PR or a few others where it was stuck).

[dougwilson]

so here is my perceived design:

no explicit setting of query parser: throw warning, one per app
explicit setting of query parser: don't throw warning, for the app that over-rode it.

is this how it should be according to you?

Yep, that sounds right. I would just want to make sure that we don't want to throw the warning though, as we want the app to continue to work without the users needing to wrap something in a try catch.

[gireeshpunathil]

I would just want to make sure that we don't want to throw the warning though, as we want the app to continue to work without the users needing to wrap something in a try catch.

deprecation warnings print informative messages and move on, they do not affect the control flow in any manner? The current change follow that model, unless I am missing something?

can you clarify that it is meant to be a statement to complete the design of deprecation, as opposed to a change request in the current PR?

[dougwilson]

deprecation warnings print informative messages and move on, they do not affect the control flow in any manner? The current change follow that model, unless I am missing something?

Right, that is what I'm looking for. You're saying that you're planning to throw the message, though, which is what I wanted to make sure was not going to happen. When you throw something in JavaScript, it does interrupt the control flow, as you need to use try catch to "catch" what was thrown.

can you clarify that it is meant to be a statement to complete the design of deprecation, as opposed to a change request in the current PR?

Appologies, I don't understand this question. Is there a different way you can word it or provide some additional clarification that may help?

[gireeshpunathil]

@dougwilson - apologies if it caused confusion.

Right, that is what I'm looking for.

actually answers my question. But to try rewording my question:

would just want to make sure that we don't want to throw the warning though

For your this comment, I meant to ask: Do you mean to say the current code is not meeting this design, or you are just stating that the control flow should not be affected? But it is clear to me now that the confusion came from my usage of the word throw, and its established usage in JS.

For me, throw warning means print a message and move on,
and throw error means alter the control flow with an error description.

but it is just me, ha ha.

[dougwilson]

This is just a question: I see you're setting to false a few lines up and then setting true here. Is there a particular reason not to just set it one time to true? Or, should the first set also be setting to true since the default state is that the default query parser is being used?

Side note as well: we may want to either make this property start with an underscore to use the denotation of "private", or perhaps use the same "symbol" tracking used with the trust proxy setting tracking (which was done in order to keep it from displaying during an iteration -- due to lack of symbol support in Node.js 0.10 that 4.x supports).

[gireeshpunathil]

@dougwilson - the idea is to use the tracker such that if the parser is set from the constructor, set it to true, else to false; so that when the middleware is created, the tracker boolean in the tracker will tell us whether an explicit call was present or not.

whether to start with a true or false, looks immaterial to me, I just set it to true, so that the value matches its name.

Also prefixed the name with an underscore to designate its private status.

[dougwilson]

Hi @gireeshpunathil sorry, that was my bad; yes, I was thinking when you say "throw" you are referring to the throw keyword, especially since you could certainly throw a warning, there is no way to understand off-hand that you didn't meant to actually use the throw keyword, my mistake. Glad we could clear that up 😃

[gireeshpunathil]

@dougwilson - review comments addressed, please have a look!

[gireeshpunathil]

@dougwilson - b311f27 : thanks for pushing, is it to satisfy some linter? if so, I see the neighbors of the changes do not follow the rule? is it that the new rule applies to new changes only?

[dougwilson]

is it to satisfy some linter? if so, I see the neighbors of the changes do not follow the rule? is it that the new rule applies to new changes only?

From https://github.com/expressjs/express/blob/master/Collaborator-Guide.md :

Follow the JavaScript Standard Style

Discussion in expressjs/discussions#70

We are currently converting the code base into Standard style by converting each line that we touch when making changes.

[dougwilson]

While I was awaiting to get clarity around the docs situation around the release processes (thanks @wesleytodd for chiming in on that on Sunday!) I had my teams test a branch of Express with this (among other things) deprecation on it. From there experience, I think we ought to forgo having this deprecation on the 4.x line and just make the 5.0 change and leave it for the migration guide.

The biggest thing raised from testing (and I can see where it comes from) is that making just a basic Express app without writing anything wrong ends up emitting the deprecation warning -- every tutorial out there, even our own getting started, etc. That is not necessarily bad, but because it's actually the omission of doing something that is the warning, it turned out it was quite confusing.

Many users have never used a setting before, and since the query parser is also a "special" setting in 4.x (fixed in 5.0 already) where you have to set it before any other middleware, routes, etc. it turned out to be even more confusing on how to "fix" the issue, as folks would have settings not necessarily at the very top.

Additionally, it also pretty much makes it even more wordy to write Express-based onliners since now you have to include the query parser setting just to get something quick done, making oneliners a bit more unwieldy.

The deprecation added to missing options in bodyParser.urlencoded and in express-session also caused a lot of opened issues and user confusion, but I think this is even more confusion than those, which were already borderline, as at least those the user had like bodyPaser.urlencoded() directly in there code where it was obvious on where to add the option to (since the message pointed to the file and line number of that call) where as this deprecation has no such hint and typically simple apps have no calls to app.set anywhere; coupled with the user has to set the query parser setting in 4.x specifically in a certain part of the calls due to the 4.x bug around it's setting.

None of this is an issue with the quality of the work done on this PR, rather feedback on the concept itself. @expressjs/express-tc may want to weigh in as well. I am very heavily learning towards this deprecation being too disruptive and suggest we skip this one.

[gireeshpunathil]

thanks @dougwilson ! user experience is extremely valuable, and it proved time and again! I can relate to all what you wrote!! kudos to your team that spent time on this, and provided candid and critical feedback!

I am happy to close this instantly on these grounds:

• deprecation printed on the most common use cases
• no obvious location visible to the user to circumvent the deprecation

but may be as you pinged the TC, let us wait for their views as well!

[dougwilson]

Yes. I am actually a bit sad, as I think deprecations are valuable, but this one turned out quite aggressive and I don't think there is any other way around it. I pinged the TC to double check mainly to give anyone a chance to advocate for the deprecation if there is a strong opinion.

I expect we will have a path forward with all these things by tomorrow night (my reference frame) to get the 4.18 PR open and, if this deprecation is not landing, the related 5.0 change merge to that respective branch.

[dougwilson]

Ok, so it is past the TC meeting, and though no TC members were present to make a decision, it has sat here for a bit for feedback, and the PR author has agreed we should not go forward with it, so I'm going to close this out instead of landing in 4.18.

Thank you for your hard work on this PR regardless.