nest
nest copied to clipboard
fix(platform): added prefix to adapters
fixes https://github.com/nestjs/nest/issues/4114
PR Checklist
Please check if your PR fulfills the following requirements:
- [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
PR Type
What kind of change does this PR introduce?
[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:
What is the current behavior?
When launching multiple NestApplication instances for a single adapter and using setGlobalPrefix
for each, the router of the instance that executed the init
method later cannot be used.
example
const server = express();
const adapter = new ExpressAdapter(server);
const adminApp = await NestFactory.create(AppModule, adapter);
const apiApp = await NestFactory.create(AppModule, adapter);
adminApp.setGlobalPrefix('admin');
apiApp.setGlobalPrefix('api');
await adminApp.init();
await apiApp.init();
server.listen(3000, () => console.log('Listening at 3000'));
result
$ curl localhost:3000/admin
admin
$ curl localhost:3000/api
{"statusCode":404,"error":"Not Found","message":"Cannot GET /api"}
Issue Number: https://github.com/nestjs/nest/issues/4114
What is the new behavior?
When setting middreware to the adapter, set the prefix registered with setGlobalPrefix
.
With the above measures, the middleware is set for each instance, and the router is set appropriately.
Does this PR introduce a breaking change?
[ ] Yes
[x] No
If we do not use setGlobalPrefix
, if we set with '/' added, if we set without '/', everything works normally.
This has been adapted to maintain compatibility.
Other information
Pull Request Test Coverage Report for Build 16357
- 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.06%) to 95.127%
Totals | |
---|---|
Change from base Build 16231: | 0.06% |
Covered Lines: | 4568 |
Relevant Lines: | 4802 |
💛 - Coveralls
Does this also still add a notFoundHandler
and an errorHandler
to the '/'
route, even when there are multiple applications being created? So in the above example there is the following handlers
Handler | Route |
---|---|
Router | admin/ |
NotFound | admin/ |
Exception | admin/ |
Router | api/ |
NotFound | api/ |
Exception | api/ |
NotFound | / |
Exception | / |
Would it be possible to keep that, as that is the current functionality? And what happens if no global prefix is set by either application? Does the first application have it's route handler overwritten?
In addition to @jmcdo29 questions, what if I have a single app + I set the global prefix to, let's say, /api
and someone tries to hit /users
without the /api
prefix. What would happen?
@jmcdo29 @kamilmysliwiec Thank you. Certainly the above is not supported. Please wait a moment as there is one idea.
Maybe we should simply add 2 additional properties to the application options interface which will determine whether to mount the "not found" and "exception handler" middleware. Hence, you would be able to compose multiple applications easily.
I changed to keep the globalprefix set by httpadapter. With this fix, NotFoundException is thrown at the right time.
example
const server = express();
const adapter = new ExpressAdapter(server);
const catsApp = await NestFactory.create(AppModule, adapter);
const dogsApp = await NestFactory.create(AppModule, adapter);
catsApp.setGlobalPrefix('cats');
dogsApp.setGlobalPrefix('dogs');
await catsApp.init();
await dogsApp.init();
server.listen(3000, () => console.log('Listening at 3000'));
result
$ curl localhost:3000/cats
Hello!
$ curl localhost:3000/dogs
Hello!
$ curl localhost:3000/cats/dogs
{"statusCode":404,"error":"Not Found","message":"Cannot GET /dogs"}
$ curl localhost:3000/dogs/cats
{"statusCode":404,"error":"Not Found","message":"Cannot GET /cats"}
$ curl localhost:3000/admin
{"statusCode":404,"error":"Not Found","message":"Cannot GET /admin"}
When prefix is not added + prefix is added, it works normally.
But it's a bit complicated
Maybe we should simply add 2 additional properties to the application options interface which will determine whether to mount the "not found" and "exception handler" middleware.
This is fine, but which one do you prefer?
This is fine, but which one do you prefer?
Example:
const server = express();
const adapter = new ExpressAdapter(server);
const catsApp = await NestFactory.create(AppModule, adapter, {
notFoundHandler: false,
});
const dogsApp = await NestFactory.create(AppModule, adapter);
catsApp.setGlobalPrefix('cats');
dogsApp.setGlobalPrefix('dogs');
await catsApp.init();
await dogsApp.init();
server.listen(3000, () => console.log('Listening at 3000'));
In this case, the dogsApp
not found handler would be used.
If one wants to disable it entirely, notFoundHandler
can be set to false
twice:
const catsApp = await NestFactory.create(AppModule, adapter, {
notFoundHandler: false,
});
const dogsApp = await NestFactory.create(AppModule, adapter, {
notFoundHandler: false,
});
What would happen if someone set notFoundHandler: true
on both? Would it come around to the same issue? Would that be something that could be warned against from the factory?
What would happen if someone set notFoundHandler: true on both? Would it come around to the same issue?
I also think the same issue occur.
Could you review the commit for b432140: fix(platform): support notfound in root url when added prefix
?
I'd like to receive your opinion and do better.
Would that be something that could be warned against from the factory?
Probably not. We can't really determine whether it was set on purpose or not. I think setting the notFoundHandler: false
twice is very unambiguous though (nobody should be surprised that neither handler was applied[?])
If catsApp is set notFoundHanlder: false
and dogsApp is set 'notFoundHanlder: true`, It will be as follows.
Handler | Route |
---|---|
Router | /cats |
NotFound | / |
Exception | / |
Router | /dogs |
So We can't get access to dogs
router.
This is because Express is handled in the order that It is set.
I will make two proposals below.
Plan 1
This add NotFoundHandler to the "/" route and prefix route.
This is the commit of b432140: fix(platform): support notfound in root url when added prefix?
.
Handler | Route |
---|---|
Router | /cats |
NotFound | /cats |
Exception | /cats |
Router | /dogs |
NotFound | /dogs |
Exception | /dogs |
NotFound | / |
-
Good Point Be compatible and behave ideally.
-
Bad Point Not simple code. (HttpAdapter needs to keep the set prefix.)
Plan 2
This add NotFoundHandler to the prefix route only.
Handler | Route |
---|---|
Router | /cats |
NotFound | /cats |
Exception | /cats |
Router | /dogs |
NotFound | /dogs |
Exception | /dogs |
And user need to add the setNotFoundHandler(callback)
to httpadapter by userself.
const server = express();
const adapter = new ExpressAdapter(server);
const catsApp = await NestFactory.create(AppModule, adapter);
const dogsApp = await NestFactory.create(AppModule, adapter);
catsApp.setGlobalPrefix('cats');
dogsApp.setGlobalPrefix('dogs');
await catsApp.init();
await dogsApp.init();
const callback = <TRequest, TResponse>(req: TRequest, res: TResponse) => {
const method = applicationRef.getRequestMethod(req);
const url = applicationRef.getRequestUrl(req);
throw new NotFoundException(`Cannot ${method} ${url}`);
};
adapter.setNotFoundHandler(callback);
server.listen(3000, () => console.log('Listening at 3000'));
-
Good Point Simple code.
-
Bad Point Incompatible.
I'm sorry. I think my way of communicating was not good.
I want to solve this problem. What should I do?
If you proceed with adding properties, there is a possibility that it can be solved by creating a new method.
Example:
const server = express();
const adapter = new ExpressAdapter(server);
const catsApp = await NestFactory.create(AppModule, adapter, {
notFoundHandler: false,
});
const dogsApp = await NestFactory.create(AppModule, adapter);
catsApp.setGlobalPrefix('cats');
dogsApp.setGlobalPrefix('dogs');
await adapter.multipleAppsInit();
server.listen(3000, () => console.log('Listening at 3000'));
What do you think?
any updates?