6 ms·

It never takes long when throwing together toy examples with these kinds of APIs to introduce a security risk: var uploads = Path.Combine(uploadsPath, file

It never takes long when throwing together toy examples with these kinds of APIs to introduce a security risk: var uploads = Path.Combine(uploadsPath, file.FileName); Where file.FileName appears to be drawn from the content-disposition header of the request. MS’s own asp.net docs on file uploads say: “Use a safe file name determined by the app. Don't use a file name provided by the user or the untrusted file name of the uploaded file.” I’m always wary of APIs designed to look good in a DevRel evangelist’s presentation, to show how you can do something in ‘just a few lines of code’, when the reality is that actual applications will need to deal with a bunch more concerns anyway and this terse little API is not going to make one jot of difference to the essential complexity you’re trying to express. Whether you have a node express style server where you’re able to declare all your controller mappings in-line using lambdas, or a spring style router where controllers are in separate classes and methods makes no real difference to the fact you still have to write the controller body, and in any real application it is not going to be handling everything from request body handling to file system operations in one place anyway. At least when the controllers are all classes I can unit test them.

deleted 5y ago
[deleted]
Zababa 5y ago
> At least when the controllers are all classes I can unit test them. You can usually very easily transform lambas in function, so testing shouldn't be a problem.
- jhgb 5y ago
  Lambas, the famous Elvish mathbread?
- jameshart 5y ago
  But if I'm not writing my controllers using inline lambda syntax, but as functions in classes in a controllers directory,, what's the value of this minimal route declaration syntax over a 'heavierweight' solution that... also puts my controller declarations in methods of classes in a controllers directory?
tester34 5y ago
>At least when the controllers are all classes I can unit test them. Do what on "controllers"??? heresy
Jenk 5y ago
> At least when the controllers are all classes I can unit test them. I was always having great difficulty testing ASP.NET MVC controllers given the use of completely closed off, cyclical dependency, sealed class, properties like HttpContext. Helper libraries made it somewhat easier to set them up but they were always heavy tests as a direct result of the large object graph needed.
- tester34 5y ago
  But why? generally you don't test controllers directly. You test "service"/"handler" classes that handle those requests. If you want to test controllers, then you write E2E tests that send HTTP requests Here's some example of how it might look like https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-5.0#sut-environment https://docs.microsoft.com/en-us/aspnet/core/test/integratio...
  - jameshart 5y ago
    If there's any critical behavior you're reliant on the controller (and, in frameworks like this, its attribute-driven configuration) to provide for you - throwing exceptions for bad requests, authentication or authorization, returning particular cache headers for particular response cases... I'd recommend looking for some way to write tests that, in isolation, go some way to let you assert those behaviors. Don't bother writing the testcase that asserts that if the controller is called with a valid body, it calls the right service and passes on the request data. Write the testcase that asserts that if it's called with a body that's way too big that it throws the appropriate 400 exception, though. One of the biggest problems with web frameworks which let you configure a lot of stuff like caching, auth and parameter validation using attributes, DI, or magic syntax, is that often the only way you can verify those behaviors are in place is with integration tests. And integration tests for failure modes are hard to write, so they don't get written. Which means it's easy to deploy a service which, say, is supposed to respond with stale cached data if a dependency is unavailable - but it actually doesn't because nobody was able to express a test case that verified that the controller caching attributes actually work the way they thought it did.
magicalhippo 5y ago
> At least when the controllers are all classes I can unit test them. So I've literally just jumped into this world, having never touched asp.net or similar before. I looked into how to test the controller side of things, and came across this[1] post arguing that unit testing controllers weren't as useful as integration testing them. Integration testing would definitely be useful, and something I will do, but I assume also a lot more time-consuming so perhaps not something we could do on a per-commit basis. Would highly appreciate views on this topic. [1]: https://andrewlock.net/should-you-unit-test-controllers-in-aspnetcore/ https://andrewlock.net/should-you-unit-test-controllers-in-a...
- achtung82 5y ago
  Integration testing as im used to it takes less time to write than unittests since you need to spend less time mocking things (mainly just mock external requests). Of course the initial setup requires more time but there are many readymade examples you can copy from. For example the official documentation. https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-5.0 https://docs.microsoft.com/en-us/aspnet/core/test/integratio...
- stonemetal12 5y ago
  Unit tests are valuable if you are doing some sort of "computation". If all code in question is doing is plugging x into y, then unit tests are useless and integration tests are where the payoff is. If you have code that does both plug and compute you should refactor them so that computing and plugging don't happen in the same place. Doing so will reduce instances of hard to test code. In general controllers are plugging code not computing code, so integration tests tend to be the more appropriate choice.
andix 5y ago
> At least when the controllers are all classes I can unit test them. There is normally not such a big benefit in unit testing them, as controllers should only call some service behind. This service you can test. Controllers are better integration tested, with a mock service behind. So you actually test if your routing and response codes are picked up correctly by the framework. There is TestServer for asp.net core. It gives you a very easy solution for integration testing, if set up correctly they are nearly as fast as plain unit tests.
- jameshart 5y ago
  If it has a mock service behind it, how is that an integration test?
  - andix 5y ago
    You test the integration of your code with the web framework. An integration test doesn't have to be end-to-end. It tests the integration between multiple components. In this case it would be your code with the integration of asp.net core. A controller usually doesn't contain any business logic, it does the HTTP part. So you test serialization, model binding, return codes, etc. So you use a mock to check, if the framework does the expected model binding. If the values you post via JSON end up in the right properties for the service call. a typical controller method would look like that: IActionResult StoreProduct(Product p) { var success = service.Store(p); return success ? Ok("Stored") : BadRequest("Product doesn't contain all necessary fields"); } There is no reason to unit test this controller, because the code is trivial. But it makes sense to integration test it, and verify the serialization and binding of Product works, if the correct status codes and content types are returned. Because that could go wrong. It is often easier to do that with a mocked service, but you can also use the real service for that. Often this would hit the database and make those tests much slower and harder to set up.
    - jameshart 5y ago
      Interesting. I would still consider that a unit test (I don’t insist that a unit test must directly call the method it’s testing - it could be it needs to put it in some sort of test harness to get it executed) Much how if a piece of code uses a regex, I wouldn’t mock the regex library when unit testing it, nor consider a test that uses the real regex library as an integration test that tests my code’s integration with the regex library, I wouldn’t consider my controller code’s interactions with a web framework as ‘integrations’ to test. They are part of how the unit is implemented. But this is just a naming debate - I’m in agreement that that is a useful kind of test to write. I just reserve ‘integration test’ for contexts where I am testing that multiple individually tested components of my own work together as they’re supposed to when combined - not that they integrate together correctly with something else I didn’t build. There’s a danger if you go down that path of ‘testing the framework’.

deleted 5y ago

[deleted]

Zababa 5y ago

> At least when the controllers are all classes I can unit test them. You can usually very easily transform lambas in function, so testing shouldn't be a problem.

jhgb 5y ago
Lambas, the famous Elvish mathbread?
jameshart 5y ago
But if I'm not writing my controllers using inline lambda syntax, but as functions in classes in a controllers directory,, what's the value of this minimal route declaration syntax over a 'heavierweight' solution that... also puts my controller declarations in methods of classes in a controllers directory?

tester34 5y ago

>At least when the controllers are all classes I can unit test them. Do what on "controllers"??? heresy

Jenk 5y ago

> At least when the controllers are all classes I can unit test them. I was always having great difficulty testing ASP.NET MVC controllers given the use of completely closed off, cyclical dependency, sealed class, properties like HttpContext. Helper libraries made it somewhat easier to set them up but they were always heavy tests as a direct result of the large object graph needed.

tester34 5y ago
But why? generally you don't test controllers directly. You test "service"/"handler" classes that handle those requests. If you want to test controllers, then you write E2E tests that send HTTP requests Here's some example of how it might look like https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-5.0#sut-environment https://docs.microsoft.com/en-us/aspnet/core/test/integratio...
- jameshart 5y ago
  If there's any critical behavior you're reliant on the controller (and, in frameworks like this, its attribute-driven configuration) to provide for you - throwing exceptions for bad requests, authentication or authorization, returning particular cache headers for particular response cases... I'd recommend looking for some way to write tests that, in isolation, go some way to let you assert those behaviors. Don't bother writing the testcase that asserts that if the controller is called with a valid body, it calls the right service and passes on the request data. Write the testcase that asserts that if it's called with a body that's way too big that it throws the appropriate 400 exception, though. One of the biggest problems with web frameworks which let you configure a lot of stuff like caching, auth and parameter validation using attributes, DI, or magic syntax, is that often the only way you can verify those behaviors are in place is with integration tests. And integration tests for failure modes are hard to write, so they don't get written. Which means it's easy to deploy a service which, say, is supposed to respond with stale cached data if a dependency is unavailable - but it actually doesn't because nobody was able to express a test case that verified that the controller caching attributes actually work the way they thought it did.

magicalhippo 5y ago

> At least when the controllers are all classes I can unit test them. So I've literally just jumped into this world, having never touched asp.net or similar before. I looked into how to test the controller side of things, and came across this[1] post arguing that unit testing controllers weren't as useful as integration testing them. Integration testing would definitely be useful, and something I will do, but I assume also a lot more time-consuming so perhaps not something we could do on a per-commit basis. Would highly appreciate views on this topic. [1]: https://andrewlock.net/should-you-unit-test-controllers-in-aspnetcore/ https://andrewlock.net/should-you-unit-test-controllers-in-a...

achtung82 5y ago
Integration testing as im used to it takes less time to write than unittests since you need to spend less time mocking things (mainly just mock external requests). Of course the initial setup requires more time but there are many readymade examples you can copy from. For example the official documentation. https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-5.0 https://docs.microsoft.com/en-us/aspnet/core/test/integratio...
stonemetal12 5y ago
Unit tests are valuable if you are doing some sort of "computation". If all code in question is doing is plugging x into y, then unit tests are useless and integration tests are where the payoff is. If you have code that does both plug and compute you should refactor them so that computing and plugging don't happen in the same place. Doing so will reduce instances of hard to test code. In general controllers are plugging code not computing code, so integration tests tend to be the more appropriate choice.

andix 5y ago

> At least when the controllers are all classes I can unit test them. There is normally not such a big benefit in unit testing them, as controllers should only call some service behind. This service you can test. Controllers are better integration tested, with a mock service behind. So you actually test if your routing and response codes are picked up correctly by the framework. There is TestServer for asp.net core. It gives you a very easy solution for integration testing, if set up correctly they are nearly as fast as plain unit tests.

jameshart 5y ago
If it has a mock service behind it, how is that an integration test?
- andix 5y ago
  You test the integration of your code with the web framework. An integration test doesn't have to be end-to-end. It tests the integration between multiple components. In this case it would be your code with the integration of asp.net core. A controller usually doesn't contain any business logic, it does the HTTP part. So you test serialization, model binding, return codes, etc. So you use a mock to check, if the framework does the expected model binding. If the values you post via JSON end up in the right properties for the service call. a typical controller method would look like that: IActionResult StoreProduct(Product p) { var success = service.Store(p); return success ? Ok("Stored") : BadRequest("Product doesn't contain all necessary fields"); } There is no reason to unit test this controller, because the code is trivial. But it makes sense to integration test it, and verify the serialization and binding of Product works, if the correct status codes and content types are returned. Because that could go wrong. It is often easier to do that with a mocked service, but you can also use the real service for that. Often this would hit the database and make those tests much slower and harder to set up.
  - jameshart 5y ago
    Interesting. I would still consider that a unit test (I don’t insist that a unit test must directly call the method it’s testing - it could be it needs to put it in some sort of test harness to get it executed) Much how if a piece of code uses a regex, I wouldn’t mock the regex library when unit testing it, nor consider a test that uses the real regex library as an integration test that tests my code’s integration with the regex library, I wouldn’t consider my controller code’s interactions with a web framework as ‘integrations’ to test. They are part of how the unit is implemented. But this is just a naming debate - I’m in agreement that that is a useful kind of test to write. I just reserve ‘integration test’ for contexts where I am testing that multiple individually tested components of my own work together as they’re supposed to when combined - not that they integrate together correctly with something else I didn’t build. There’s a danger if you go down that path of ‘testing the framework’.