Open guilhermeluizsp opened 4 years ago
Hello @guilhermeluizsp I was wondering about your comment saying that LightController forcefully returns a DomainResponse structure because there is a way to return a no content response.
The LightController has no way to find out by itself what it should response, anyway, it seems possible to me if I inform the StatusCode as 204 on DomainResponse.
LightController:
One approach would be to modify the CarController class.
CarController:
With this, the returns are expected:
Another approach would be making the Service layer decides on it...but for me it sounds wrong:
@moraes-caroline-avanade
The LightController has no way to find out by itself what it should response
I'm not sure if I understood this statement correctly, but LightController
does know what to return based on the response provided. The Result
method does exactly that. It returns BadRequest when BadRequestMessage
is true
, for example. Returning a "no content" result is no different and your solution does work that way (and it logic is correct btw).
Another approach would be making the Service layer decides on it...but for me it sounds wrong.
You're completely right, it is indeed wrong, but if we take the current version of Liquid into account, we can see that they're coupled already (see #34). I can attest that with this line of code:
DomainResponse
's StatusCode
is an HTTP concept so, as aforementioned, your solution seems to be the most appropriate.
@guilhermeluizsp ,
I apologize, I edited my first comment, could you please comment if I should proceed with some of the solutions that are present, in the first or second?
I'm not sure if I understood this statement correctly, but LightController does know what to return based on the response provided. The Result method does exactly that.
When mentioned that the method cannot find out, I was referring to the method that does not identify which is the HTTP verb, specified DELETE or PUT that should return the result of statusCode 204 - No Content.
@moraes-caroline-avanade This implementation looks good enough for the current scenario of Liquid (you should move this implementation upwards, and consider the nullability behavior of the StatusCode
property, of course):
@bruno-brant @bavanade I'd implement this fix with a factory instead of putting directly into the Result
method. Do you think it's worth it?
Hey @guilhermeluizsp could you elaborate? Is the factory supposed to replace the whole behavior of Result? I.e., a transformer between DomainResult
and IActionResult
?
Hey @guilhermeluizsp could you elaborate? Is the factory supposed to replace the whole behavior of Result? I.e., a transformer between
DomainResult
andIActionResult
?
Exactly that.
@bruno-brant Of course we'd make sure this factory is extensible enough to make people return whatever they want, but the ultimate goal is to transform a DomainResponse
into an IActionResult
It seems like a good idea - for now, how do we wire it up with LightController
? Optional injection?
@bruno-brant Yes. By using this we'd also allow people to override it for custom scenarios.
Though this would be an excellent feature for Liquid, it would go against the proposed solution here: Just add another branch do the existing Result
method. Do you think we should go for this simplistic approach for now, or should we skip it and aim directly into the factory one?
For the sake of agility (real agility, no scrum-certified, etc.), let's just do this as it is and immediately create an enhancement to encapsulate this behavior in a factory. Alright, @moraes-caroline-avanade?
The section about the status code 204 (no content) in RFC 7231 explicitly says that this kind of response cannot contain a message body. ASP.NET Core's Kestrel Web Server correctly validates this behavior by throwing an exception.
The method
Result
fromLightController
does not take this scenario into account and forcefully returns aDomainResponse
structure:https://github.com/Avanade/Liquid-Application-Framework/blob/94071135ae594260cdb6f6264912492a104b1420/src/Liquid.Activation/Controller/LightController.cs#L63-L87