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

After migrating to v5 from v4 the matching service name collides with the rest defined endpoints. #3553

Open
fragilehm opened this issue Dec 2, 2024 · 3 comments

Comments

@fragilehm
Copy link

Description

It is not really an issue, but something I have faced while upgrading from version 4 to version 5.

In version 4 I have a service named items, with some custom events:

const createService = require("feathers-memory");
module.exports = function (app) {
  class Items {
    constructor() {
      this.items = [];
    }
  }

  // Initialize our service with any options it requires
  app.use(
    "/items",
    createService({
      events: [
        ...custom_events
      ],
      Items,
    }),
  );
};

I also had the rest endpoint defined:

app.post("/items/new", (req, res) => {
  const { body } = req;
  const itemsService = app.service("items");
  itemsService.emit("new_item", body);
  res.status(200);
  res.json({
    data: "Success",
  });
});

So when calling the app with items/new endpoint it will go to the defined rest endpoint handler and emit the event.

After upgrading to v5, the services file was updated to:

import { Application, Params } from "@feathersjs/feathers";

export default function (app: Application) {
  class ItemsService {
    async create(_data: any, _params: Params) {
      return [];
    }
  }

  // Initialize our service with any options it requires
  app.use("/items", new ItemsService(), {
    events: [
      ...custom_events
    ],
  });
}

and the rest endpoint definition is the same as before, but the issue now is when calling a post request with items/new

the service create is being called not the rest defined endpoint.

Is there a way to explicitly set the priority for the rest defined endpoint?

Just renaming a service is an option but in this case, the relevant clients have to update the service names as well.

@JulienZD
Copy link
Contributor

JulienZD commented Jan 3, 2025

We are experiencing the same issue. We only just now got around to upgrading to v5. We have roughly 20 routes like this that are now broken. Some of these are used by external API consumers, so we can't easily redefine these, as that would be considered a breaking change.

I did some testing and it appears to only be an issue with @feathersjs/express, as the feathers-chat-ts example is not affected with @feathersjs/koa. From what I can tell there's no easy way to use the same routing logic with Koa (I briefly tried with @koa/router, but couldn't get it to work). Migrating to Koa would be a lot of work for us too..

I tried to investigate a bit, it seems that the route is being detected too eagerly in the @featherjs/transport-commons lookup logic.

It only seems to affect custom Express routes, any normal service registered under the same path gets routed properly.

Small reproduction / investigation logs

// feathers-chat-ts/src/services/users/users.ts
export const user = (app: Application) => {
  console.log('feathers-chat - registering: /users/test')
  app.use('/users/test', (req, res) => {
    res.json({ message: 'Hello from the user service' })
  })

  console.log('feathers-chat - registering: /users')

  app.use('users', new UserService(getOptions(app)), {
    methods: userMethods,
    events: []
  })
}

Output of DEBUG=express:router pnpm dev with internal logs added to:

  • @featherjs/transport-commons/lib/routing/index.js -> routing()
  • @feathersjs/feathers/lib/application.js -> Application['use']
  • @feathersjs/express/lib/rest.js -> servicesMiddleware.toHandler
  express:router use '/' query +0ms
  express:router use '/' expressInit +1ms
  express:router use '/' <anonymous> +0ms
  express:router use '/' corsMiddleware +0ms
  express:router use '/' serveStatic +0ms
  express:router use '/' <anonymous> +0ms
  express:router use '/' <anonymous> +0ms
  express:router use '/' <anonymous> +0ms
@feathersjs/feathers - app.use: { path: 'messages', type: 'object' }
@feathersjs/transport-commons -  { path: 'messages', params: {} }
  express:router use '/' <anonymous> +13ms
  express:router use '/' formatter +1ms
feathers-chat - registering: /users/test
  express:router use '/users/test' <anonymous> +0ms
feathers-chat - registering: /users
@feathersjs/feathers - app.use: { path: 'users', type: 'object' }
@feathersjs/transport-commons - registering { path: 'users', params: {} }
  express:router use '/' <anonymous> +0ms
  express:router use '/' formatter +0ms
info: Feathers app listening on http://localhost:3030
@feathersjs/transport-commons lookup { path: 'authentication', result: null }
  express:router dispatching GET /users/1 +905ms
  express:router query  : /users/1 +2ms
  express:router expressInit  : /users/1 +1ms
  express:router <anonymous>  : /users/1 +0ms
  express:router corsMiddleware  : /users/1 +0ms
  express:router serveStatic  : /users/1 +0ms
  express:router <anonymous>  : /users/1 +2ms
  express:router <anonymous>  : /users/1 +0ms
  express:router <anonymous>  : /users/1 +0ms
@feathersjs/transport-commons lookup {
  path: '/users/1',
  result: {
    params: { __id: '1' },
    data: { service: [UserService], params: {} }
  }
}
@feathersjs/express lookup {
  path: '/users/1',
  lookup: { service: 'object', params: { __id: '1' } }
}
  express:router dispatching GET /users/1 +3ms
  express:router <anonymous>  : /users/1 +0ms
  express:router formatter  : /users/1 +2ms
  express:router dispatching GET /users/test +7s
  express:router query  : /users/test +0ms
  express:router expressInit  : /users/test +0ms
  express:router <anonymous>  : /users/test +0ms
  express:router corsMiddleware  : /users/test +0ms
  express:router serveStatic  : /users/test +0ms
  express:router <anonymous>  : /users/test +1ms
  express:router <anonymous>  : /users/test +0ms
  express:router <anonymous>  : /users/test +0ms
@feathersjs/transport-commons lookup {
  path: '/users/test',
  result: {
    params: { __id: 'test' },
    data: { service: [UserService], params: {} }
  }
}
@feathersjs/express lookup {
  path: '/users/test',
  lookup: { service: 'object', params: { __id: 'test' } }
}
  express:router dispatching GET /users/test +0ms
  express:router <anonymous>  : /users/test +0ms
  express:router formatter  : /users/test +1ms

It seems like the route is being registered correctly, but the lookup fails. If I remove the app.use('users', new UsersService()) line, the logs look like this:

  express:router use '/' query +0ms
  express:router use '/' expressInit +0ms
  express:router use '/' <anonymous> +0ms
  express:router use '/' corsMiddleware +0ms
  express:router use '/' serveStatic +1ms
  express:router use '/' <anonymous> +0ms
  express:router use '/' <anonymous> +0ms
  express:router use '/' <anonymous> +0ms
@feathersjs/feathers - app.use: { path: 'messages', type: 'object' }
@feathersjs/transport-commons - registering { path: 'messages', params: {} }
  express:router use '/' <anonymous> +11ms
  express:router use '/' formatter +0ms
feathers-chat - registering: /users/test
  express:router use '/users/test' <anonymous> +1ms
info: Feathers app listening on http://localhost:3030
@feathersjs/transport-commons lookup { path: 'authentication', result: null }
  express:router dispatching GET /users/test +2s
  express:router query  : /users/test +1ms
  express:router expressInit  : /users/test +1ms
  express:router <anonymous>  : /users/test +1ms
  express:router corsMiddleware  : /users/test +0ms
  express:router serveStatic  : /users/test +0ms
  express:router <anonymous>  : /users/test +2ms
  express:router <anonymous>  : /users/test +1ms
  express:router <anonymous>  : /users/test +0ms
@feathersjs/transport-commons lookup { path: '/users/test', result: null }
@feathersjs/express lookup {
  path: '/users/test',
  lookup: { sercice: 'undefined', params: undefined }
}
  express:router trim prefix (/users/test) from url /users/test +2ms
  express:router <anonymous> /users/test : /users/test +0ms

Here the @feathersjs/transport-commons lookup doesn't find anything, so it probably continues onto the express lookup, which does find it. Could it be related to this line registering a catch-all ${path}/:__id route which takes priority over the specific express defined route?

Update - hacky workaround

I found a workaround that works for regular GET calls (find in feathers), which is our only use case.

It seems that adding an empty Feathers service object in between causes the route to be registered properly. Then you can define the Express handler as the third argument (which will be an express middleware, but you can just respond here as well):

  app.use(
    '/users/test',
+    {
+     async find() {
+      return {};
+      },
+    },
    (req, res) => {
      // Express implementation
    }
  );

  app.use('users', new UsersService());

@JulienZD
Copy link
Contributor

JulienZD commented Jan 6, 2025

Small update: I made a reproduction which illustrates the routing breaking between v4 and v5.

I realized that the hacky workaround I posted in my previous comment doesn't actually work all too well, since the hooks from the base service are now always being called as well for the nested endpoint. This breaks some of our endpoints, since they don't contain the expected data/params by the hooks

@JulienZD
Copy link
Contributor

JulienZD commented Jan 6, 2025

Another update: there was an issue with my hacky workaround to pass an empty service, as that causes all requests to that endpoint to first go through all the global app hooks (before, after, around)

I found another workaround that seems to result in the behavior of v4, by making use of the express.before options alongside an empty service, which will be executed before the serviceMiddleware from @feathersjs/express/rest (as seen here), skipping the feathers hook flow entirely:

  app.use('/users/test', createEmptyPassthroughService(), {
    express: {
      before: [
        (req, res, next) => {
          // Optionally do whatever in the first middleware
          next()
        },
        (req, res) => {
          res.json({ message: 'Hello from express endpoint /users/test' })
        }
      ]
    }
  });

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

No branches or pull requests

2 participants