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

Route-Exclusive Custom Constraint Strategy #311

Open
joelnet opened this issue Dec 1, 2022 · 4 comments
Open

Route-Exclusive Custom Constraint Strategy #311

joelnet opened this issue Dec 1, 2022 · 4 comments

Comments

@joelnet
Copy link

joelnet commented Dec 1, 2022

First of all, cheers 🍻 to all the maintainers of this amazing project. Thank you for all your hard work!

The documentation states:

Constraint strategies should be careful to make the deriveConstraint function performant as it is run for every request matched by the router.

I am seeking a way to limit the impact of the deriveConstraint function to only the routes in need of the constraint. This has been previously discussed #165.

The goals:

  1. Constraints should not be run on non-constrained routes.
  2. If a constraint fails, router should fallback to next matching route.

I did create a hackish solution that involves keeping a second router as an allow list. It works, but I am not completely in love with this.

The solution relies on some Fastify methods, so example isn't limited to find-my-way.

// @ts-check
import Fastify from "fastify";
import fp from "fastify-plugin";
import FindMyWay from "find-my-way";

const port = 8888;
const host = "0.0.0.0";

/**
 * @param {import('fastify').FastifyInstance} fastify
 */
const featurePlugin = fp(async (fastify) => {
  async function expensiveAsyncCall() {
    console.log("!!! expensiveAsyncCall !!!");
    return true;
  }

  const routesAllow = FindMyWay();

  /** @type {any} */
  const featureConstraint = {
    // strategy name for referencing in the route handler `constraints` options
    name: "feature",
    // storage factory for storing routes in the find-my-way route tree
    storage() {
      let state = {};
      return {
        get: (key) => {
          return state[key] || null;
        },
        set: (key, store) => {
          state[key] = store;
        },
      };
    },
    // function to get the value of the constraint from each incoming request
    deriveConstraint(req, ctx, done) {
      const route = routesAllow.find(req.method, req.url);
      if (!route) return done(null, null);

      expensiveAsyncCall().then(
        (data) => done(null, data),
        (err) => done(err, null)
      );
    },
  };

  fastify.addHook("onRoute", (route) => {
    if (route.constraints && featureConstraint.name in route.constraints) {
      // Create Mock Route for "find" in deriveConstraint.
      routesAllow.on(route.method, route.path, () => {});
    }
  });

  fastify.addConstraintStrategy(featureConstraint);
});

async function main() {
  const fastify = Fastify();
  await fastify.register(featurePlugin);

  // Constraint should NOT be run here
  fastify.get("/always", () => {
    return { route: "/always" };
  });

  fastify.get("/sometimes", { constraints: { feature: true } }, () => {
    return { route: "/sometimes" };
  });

  // Constraint should NOT be run here
  fastify.get("*", () => {
    return { route: "*" };
  });

  await fastify.listen({ port, host });

  console.log(`Listening on http://${host}:${port}/`);
  console.log(`             http://${host}:${port}/always`);
  console.log(`             http://${host}:${port}/sometimes`);
  console.log(`             http://${host}:${port}/catchall`);
}

main().catch((err) => {
  console.error(err);
  process.exit(1);
});

The above code has a function expensiveAsyncCall that is only run for the /sometimes route.

if feature is set to false or expensiveAsyncCall returns false, the route will fallback to the "*" route.

I am not sure exactly what I am seeking here. Feedback? Direction? Help?

@ivan-tymoshenko
Copy link
Collaborator

ivan-tymoshenko commented Dec 2, 2022

Hi, thank you for your proposal. It makes much more sense since we added async constraints, and I think we should add it. Although it wouldn't be an easy PR for a couple of reasons:

  1. Sync constraints should be as fast as they are.
  2. We shouldn't call the same strategy a second time if the first route fails. We should cache these values.
  3. find, and lookup methods should have the same interfaces and expected behavior unless we decide to release a new major for this feature.

It might take some time to add this.

@joelnet
Copy link
Author

joelnet commented Dec 2, 2022

I briefly looked at the source and it does indeed look like a complicated change. The complexity is mostly due to the constraints being fully derived prior to call to find.

  /** code has been simplified **/
  this.constrainer.deriveConstraints(req, ctx, (err, constraints) => {
      const handle = this.find(req.method, req.url, constraints)
  })

It looks like find (and thus HandlerStorage) would have to be modified to support an asynchronous, on-the-fly lookup of constraints.

@SgtPooki
Copy link

SgtPooki commented Oct 26, 2023

From reading all the docs, it seemed like a constraint that failed validate would not apply to a route. Unfortunately, this is not the case and the whole server startup fails.

@ivan-tymoshenko
Copy link
Collaborator

From reading all the docs, it seemed like a constraint that failed validate would not apply to a route. Unfortunately, this is not the case and the whole server startup fails.

  1. Can you point to the place in the docs where it's said.
  2. It sounds like a completely different problem, so open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants