Wrong queryString when building navigation plan in aurelia-router

[MartyBoi]

I'm submitting a bug report

• Library Version:
  1.7.1

Please tell us about your environment:

• Operating System:
  Windows 10

• Node Version:
  10.15.1

• NPM Version:
  6.4.1

• JSPM OR Webpack AND Version
  webpack 4.29.6

• Browser:
  all

• Language:
  TypeScript

Current behavior:
Not redirected with correct query parameter. We are using aurelia-open-id-connect and when redirected back from our identity provider the query parameters are removed when creating the redirectPlan in aurelia-router, navigation-plan.ts.
In my example you can see that the path we're navigating to is "/example?test=123". But in the end the query string is removed because the wrong instruction is used when getting query params.

• What is the expected behavior?
  Instead of getting queryString from instruction like this:

if (instruction.queryString) {
    redirectLocation += '?' + instruction.queryString;
}

I think you should get it from redirectInstruction like this:

if (redirectInstruction.queryString) {
    redirectLocation += '?' + newInstruction.queryString;
}

[bigopon]

@MartyBoi thanks for filing this issue. Any chance you could help with a minimal repro? Maybe just even pseudo code

[MartyBoi]

@bigopon of course! Is this ok?

redirected from external site, to e.g "https://exampleurl.com/routePage?queryTest=value";
router captures with query string included;
router redirects to "routePage" with no query string, e.g "https://exampleurl.com/routePage";

Expected behavior:

redirected from external site, to e.g "https://exampleurl.com/routePage?queryTest=value";
router captures with query string included;
router redirects to "routePage" with query string included, e.g "https://exampleurl.com/routePage?queryTest=value";

[bigopon]

@MartyBoi I tried to reproduce your issue with this test 24394dd#diff-c70fb785178d11245e55f0a186031839R14

But it seems it's working fine. I think we may need your help with the route configuration here 24394dd#diff-a24a48cd3ab9454455fb132bcc4442d1R9 to better show the bug.

[MartyBoi]

I think the router should be like this:

configureRouter(config: RouterConfiguration, router: Router) {
    config
      .map([
        { route: ['', 'home'], name: 'home', moduleId: PLATFORM.moduleName('./landing') },
        { route: 'routePage', name: 'route.page', moduleId: PLATFORM.moduleName('./search') },
        { route: 'redirectPage', name: 'redirect', moduleId: PLATFORM.moduleName('./someExistingModule), redirect: '/routePage?queryTest=123'
      ])
      .mapUnknownRoutes(instruction => {
        return { redirect: 'routePage' };
      });

    this.router = router;
  }

If you now try to navigate to 'redirectPage' it will not include the query string when redirected to 'routePage?queryTest=123'.

[bigopon]

@davismj I think we should merge the query param from both instruction, with redirect instruction query string overriding the current instruction query string. This requires deserializing/serializing query string of both instructions. Thoughts?

We have existing tests verifying that redirect should get the query string from current instruction, so it makes this complicated.

[MartyBoi]

@bigopon Any progress on this? Do you want me to create a pull request for this and merge the query params from both instruction?

[MartyBoi]

@bigopon I think something like this should keep your existing tests intact and fix the issue I'm having. Could you please try this?

let queryString = instruction.queryString;
if (queryString) {
   redirectLocation += '?' + queryString;
}
let redirectQueryString = redirectInstruction.queryString;
if (redirectQueryString) {
   if (queryString) {
      redirectLocation += redirectQueryString;
   } else {
      redirectLocation += '?' + redirectQueryString;
   }
}

[minimoe]

@bigopon @davismj had you had a chance to look into this? Thanks

[davismj]

I haven't. Am I correct to say that the issue is you're redirecting from page a to page b and the query string from page a is lost on page b? That sounds like a bug.

If someone could help me get started by creating a PR for a failing test, that would really help me jump in and fix this.

[jlipford]

Anyone have workaround for this?

[rmja]

@davismj There are simple failing examples in #667 and #643 seems to solve the issue.