[BUGFIX lts] Router service: Failing with query parameters on application controller

[st-h]

This PR should fix #16594 where an ember application would break when there is a query param on a parent route and another transition is triggered (for instance a redirect to a login page).

❗️ This PR should be reviewed by someone who is familiar with the details of the routing implementation

Additionally we probably should think about refactoring the _hydrateUnsuppliedQueryParams implementation, as I found it very difficult to figure out what the exact intent of this method and its assertions are. And I just added another conditional in trying to fix this, which makes things just a little worse.

Furthermore, my changes are based on assumptions I drew from current tests, which are:

• the assertion should only be triggered when there are aliases to query params
• when there is no alias: qp.urlKey == qp.prop

In case these assumptions are correct, I think we should clarify the message of the assertion. In case you run into this assertion the message does not make it very clear what actually is going on. And you find yourself in a sort of vacuum very quickly, where there are very, very little resources available that would help to figure things out.

[st-h]

Is there anything else I can do to help get this issue fixed?
I would really be a relieve to get this sorted out, because it causes ember apps to break on patch releases of add ons, which is quite hard to control reliably. There is not much addon authors can do, since technically their addons are behaving correctly - the actual root cause lies within the ember routing code.

Does anyone have any experience with the browserstack test, as it times out without any reason. From the log output all previous tests pass:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

The build has been terminated

[rwjblue]

Looks good to me. Thank you for digging into this!

BRW, I totally agree with you that this should ultimately be significantly refactored. In the end I think we’d want to completely remove this method (the router service specifically does not rehydrate values for query params that are not specified so when we move tonusing the router service primarily we should be able to kill this codepath entirely).

[rwjblue]

Restarted the cross browser tests, but we might need to rebase this to pull in some fixes for Edge that landed a few days ago.

[rwjblue]

Ya, will need a rebase sorry. Could you also update the commit message to start with [BUGFIX lts]? That will help us ensure this gets backported into 3.12 (and possibly 3.8).

[jrjohnson]

I'm trying to track down some test failures I'm seeing with my app in 3.13-beta.3 and this is a strong candidate. Will it be able to be included in a beta before 3.13 is finalized?

[st-h]

seriously... I really do not want to complain and stuff, but why does it take over a month to merge a regression which should have been fixed a long long time ago into the code base? I would really like to help move this forward somehow, but I really don't know how anymore. Please let me know what I can do to help to at least make this available within a canary version. Right now this has been a very frustrating experience - and I kind of start to feel guilty of encouraging others to get involved in ember development because this kind of experience is nothing I want to put anyone through within their spare time.

[pzuraq]

@st-h I'm really sorry about the lead time on this! The issue is there aren't that many folks who have enough context to fully review this change (QPs are very nuanced, and not many folks know how they're supposed to work throughout the whole system) and we're all very busy with trying to ship Octane at the moment. This is definitely still in the queue, and we really appreciate your patience and effort to push this through.

[st-h]

@pzuraq thanks for your reply on this. So, the actual issue is that we need more reviews first, before we can try to ship this?

[pzuraq]

Effectively, yes. I discussed this with @rwjblue the other day and he expressed that he wanted to give it one more review before merging, and I know he's busy at the moment between EmberCamp and Octane blockers. I tried to review it myself, but it's hard for me to reason about the what the assertion is trying to do (as you pointed out in your initial message on this PR, the intent of the method is hard to understand in isolation). If I get more time this week, I'll try to give it another shot, but if not it'll definitely get reviewed once we're able to get Octane shipped.

[st-h]

@pzuraq thanks for clarifying. I was under the impression that the review would have been sufficient. We just need to be super careful when updating our dependencies, as this issue tends to show up when upgrading patch versions and often only shows up on edge cases which are very easy to miss unless test coverage is very good. This somewhat feels like a ticking time bomb hidden in our ember app.

[rwjblue]

Did more deep diving (into some other production apps using query params) to confirm that this is the right path forward. Sorry that extra digging took so long 😢.

[rwjblue]

Cherry-picked into release branch (for release in 3.14.1), and once that has been out for a bit we'll make a 3.12 patch release.

[Samsinite]

Would be super happy if this also made it into 3.8 :)

[rwjblue]

3.8 is only receiving security fixes at this point, we will likely get a 3.12 patch release out with this shortly after 3.14's completely released.