LightController's Result method won't correctly handle "no content" results

[guilhermeluizsp]

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 from LightController does not take this scenario into account and forcefully returns a DomainResponse structure:

https://github.com/Avanade/Liquid-Application-Framework/blob/94071135ae594260cdb6f6264912492a104b1420/src/Liquid.Activation/Controller/LightController.cs#L63-L87

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:

[guilhermeluizsp]

@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:

https://github.com/Avanade/Liquid-Application-Framework/blob/499b418c2e852ddfa50b296aacd7d17cd2c7acc1/src/Liquid.Activation/Controller/LightController.cs#L84

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.

[guilhermeluizsp]

@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?

[bruno-brant]

Hey @guilhermeluizsp could you elaborate? Is the factory supposed to replace the whole behavior of Result? I.e., a transformer between DomainResult and IActionResult?

[guilhermeluizsp]

Hey @guilhermeluizsp could you elaborate? Is the factory supposed to replace the whole behavior of Result? I.e., a transformer between DomainResult and IActionResult?

Exactly that.

[guilhermeluizsp]

@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

[bruno-brant]

It seems like a good idea - for now, how do we wire it up with LightController? Optional injection?

[guilhermeluizsp]

@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?

[bruno-brant]

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?