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

ignoreTrailingSlash doesn't work with wildcard routes #3331

Closed
2 tasks done
moonmeister opened this issue Sep 21, 2021 · 14 comments · Fixed by #3846
Closed
2 tasks done

ignoreTrailingSlash doesn't work with wildcard routes #3331

moonmeister opened this issue Sep 21, 2021 · 14 comments · Fixed by #3846
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@moonmeister
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

3.21.1

Plugin version

4.2.3

Node.js version

14.17.6

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

11.6

Description

I want to listen on all routes under /app including /app and /app/anything.

I want to serve the same html file for any of these routes.

fastify.get(
        "/app/*,
        {
          exposeHeadRoute: true,
          prefixTrailingSlash: 'slash',
        },
        (_req, reply) => {

          reply.sendFile("index.html", path.resolve("./public", p.path.replace("/", "")));

        },
      );

This works for /app/ and /app/:anything.

I originally tried to setup a redirect then from /app => /app/ but because I had ignoreTrailingSlash enabled this created an infinite loop. Basically the /app didn't match /app/ but for some reason /app/ did match /app even though the redirect was configured with prefixTrailingSlash: "no-slash".

My guess was that this was being caused by ignoreTrailingSlash being applied to the redirect but not the initial catch all.

Disabling ignoreTrailingSlash did fix my redirect. But I'd rather not have to redirect, I'd rather just have /app currectly resolve as a 200.

I then tried:

fastify.get(
        "/app*,
        // all the smae

This correctly resolves everything: /app, /app/, and /app/:anything.

Steps to Reproduce

I think above covers this. Let me know if you need more info.

Expected Behavior

I'd expect /app/* to resolve /app, /app/, and /app/:anything when ignoreTrailingSlash: true and only /app/, and /app/:anything when ignoreTrailingSlash: false .

@mcollina
Copy link
Member

Would you like to work on this bug? How would you fix it?

@moonmeister
Copy link
Author

I can probably work on it. Haven't delved into the code to figure out where the issue lies. I guess I wanted to confirm what I expected to happen was reasonable. Sounds like you agree, any suggestions on where to start looking in the code?

@mcollina
Copy link
Member

I agree that there needs to be a way to make work what you are trying to do. However I'm not sure about the actual fix

I originally tried to setup a redirect then from /app => /app/ but because I had ignoreTrailingSlash enabled this created an infinite loop. Basically the /app didn't match /app/ but for some reason /app/ did match /app even though the redirect was configured with prefixTrailingSlash: "no-slash".

I don't understand this at all.

I think the best next step is to write a small example with a few example http calls to explain what you would like to achieve.

@moonmeister
Copy link
Author

Here's an example server with tests:

import Fastify from "fastify";
import fetch from "node-fetch";

const fastify = new Fastify({ ignoreTrailingSlash: true });

fastify.get("/test/:stuff", (req, reply) => {
  console.log("Request from ", req.url);
  reply.send(`ok! ${req.params.stuff}`);
});

fastify.listen(3000, async (err, address) => {
  if (err) throw err;
  console.log(`server listening on ${address}`);

  for (let path of ["/test/", "/test/example", "/test"]) {
    console.log(`Testing GET ${path}`);

    const res = await fetch(`http://localhost:3000${path}`);

    if (res.status >= 200 && res.status < 300) {
      const body = await res.text();
      console.log(`${res.url} passed: ${res.status}`);
      console.log(`response body: ${body}`);
    } else {
      console.log(`${res.url} failed: ${res.status}`);
      console.log(`This is expected to work when ignoreTrailingSlash:true`)
    }
  }
});

@climba03003
Copy link
Member

I do not see it will create a infinite loop from your example.

Some snippets below and full repro and test case on replit

const Fastify = require("fastify");

const fastify = Fastify();

fastify.get("/test", (req, reply) => {
  reply.redirect("/test/");
});

fastify.get("/test/*", (req, reply) => {
  reply.send(`ok! ${req.params.stuff}`);
});
  1. /test is properly redirect to /test/ and no loop back to /test.
  2. /test/ and /test/* provide content correctly.

@moonmeister
Copy link
Author

Now enable { ignoreTrailingSlash: true })

@RafaelGSS
Copy link
Member

I'm not sure if it is expected to work when ignoreTrailingSlash = true in my point of view it should match only when:

  • /test/
  • /test/example
  • /test/example/

@moonmeister
Copy link
Author

I'm not sure if it is expected to work when ignoreTrailingSlash = true in my point of view it should match only when:

  • /test/
  • /test/example
  • /test/example/

This is fine if ignoreTrailingSlash = false.

But the I think it should also have - /test when ignoreTrailingSlash = true. Even if we decide this isn't the pattern we want I should be able to add the redirect...but that loops. I'll work on a demo for that.

@climba03003
Copy link
Member

climba03003 commented Sep 24, 2021

I'm not sure if it is expected to work when ignoreTrailingSlash = true in my point of view it should match only when:

  • /test/
  • /test/example
  • /test/example/

This is fine if ignoreTrailingSlash = false.

But the I think it should also have - /test when ignoreTrailingSlash = true. Even if we decide this isn't the pattern we want I should be able to add the redirect...but that loops. I'll work on a demo for that.

I think your assumption for ignoreTrailingSlash is wrong.

/test should never be able to redirect to /test/, it is by design they are identical if ignoreTrailingSlash is enable.
To make it works you need to redirect to the path like /test/<something>, it should not be empty.

More clear image for how /test with ignoreTrailingSlash works

const Fastify = require("fastify");

const fastify = Fastify({ ignoreTrailingSlash: true });

fastify.get("/test", (req, reply) => { reply.redirect("/test/"); });

// `/test/` will throw because it is registered at the same time of `/test` when `ignoreTrailingSlash` is enabled
fastify.get("/test/", (req, reply) => { reply.send(`ok! ${req.params.stuff}`); });

fastify.get("/test/*", (req, reply) => { reply.send(`ok! ${req.params.stuff}`); });

@moonmeister
Copy link
Author

How I want my API to work is my business. I need /test and /test/ to be the same, and have the option of passing in a route param. I just want to be able to implement it. Right now I can't. https://replit.com/@moonmeister/fastify-3331 show's why not.

@climba03003
Copy link
Member

climba03003 commented Sep 24, 2021

  1. The nature of ignoreTrailingSlash are auto register /test when you registered /test/ and auto register /test/ when you registered /test.

  2. I don't know why you need to .redirect('/test/') for both /test and /test/ (is it what you means the same?) If not shouldn't the below one enough?

const Fastify = require("fastify");

const fastify = Fastify();

fastify.get("/test", (req, reply) => {
  reply.redirect("/test/");
});

fastify.get("/test/*", (req, reply) => {
  reply.send(`ok! ${req.params.stuff}`);
});

if you need /test/ to be special, just add one more route.

const Fastify = require("fastify");

const fastify = Fastify();

fastify.get("/test", (req, reply) => {
  reply.redirect("/test/");
});

fastify.get("/test/", (req, reply) => {
  reply.send('I am special')
});

fastify.get("/test/*", (req, reply) => {
  reply.send(`ok! ${req.params.stuff}`);
});

@moonmeister
Copy link
Author

moonmeister commented Sep 24, 2021

const Fastify = require("fastify");

const fastify = Fastify();

fastify.get("/test", (req, reply) => {
  reply.redirect("/test/");
});

fastify.get("/test/*", (req, reply) => {
  reply.send(`ok! ${req.params.stuff}`);
});

yes, this SHOULD work. But as the repl shows as soon as you add ignoreTrailingSlash it stops working because the /test redirect ignores the trailing slash and now applies to /test/ too. My entire application needs ignoreTrailingSlash so I can't disable it. It breaks my ability to do this redirect but doesn't just automatically handle /test so i need a redirect...but that's broken...I tried using { prefixTrailingSlash: "no-slash" } on the Redirect to force it to only redirect on /test and not /test/ but that too does nothing. again over riden by ignoreTrailingSlash.

The only thing that might work now would be :

const Fastify = require("fastify");

const fastify = Fastify();

const myHandler = (req, reply) =>  {
 const stuff = req?.params?.stuff;
  reply.send(`ok! ${stuff}`);
}

fastify.get("/test/", myHandler);
});

fastify.get("/test/*", myHandler);

But this seemed overly verbose and frustrating. /test* does work but I'm receiving /test/* from a system I can't control. This means I have to modify the received string from /test/* => /test*. This really isn't a big deal...but it seemed like this should just work and not be so complicated. Thus the bug report.

@climba03003
Copy link
Member

climba03003 commented Sep 24, 2021

When you understand the nature of ignoreTrailingSlash. It is very clear why your problem appear and all behavior is well documented.

  1. ignoreTrailingSlash is the option of find-my-way, the additional route is added in there.
  2. prefixTrailingSlash is the option of fastify, it cannot override the ignoreTrailingSlash setting and it is used when the path is inside plugin. https://github.com/fastify/fastify/blob/main/docs/Routes.md#handling-of--route-inside-prefixed-plugins

If it really need a fix to the problem of mixing ignoreTrailingSlash and prefixTrailingSlash. The behavior of ignoreTrailingSlash should be completely changed.

Maybe a more clear document of this two option is needed?

@moonmeister
Copy link
Author

Sure, docs are always helpful. I can appreciate as someone who's intimately familiar with how fastify is built that this makes sense. To everyone who uses this package this is all fastify configs that should be modifying the same thing and aren't. Either way. That was just one attempt I made at resolving the underlying issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants