jgauffin / Griffin.Framework

Application framework for Business Applications
http://griffinframework.net
168 stars 62 forks source link

HttpMessageDecoder: use of message serializer #6

Closed wo80 closed 8 years ago

wo80 commented 9 years ago

In the OnRequestLine method there are checks for _messageSerializer == null, but the message serializer isn't used at all. It's a bit confusing to get a HttpRequestBase in the one case, but a HttpRequest in the other.

Is there a reason for this?

crisim commented 9 years ago

you should look in the TriggerMessageReceived method, there the _message needs to be a HttpRequest to be processed by the _messageSerializer

wo80 commented 9 years ago

I don't see why you should use HttpRequestBase at all:

In OnRequestLine:

_message = _messageSerializer != null
                    ? new HttpRequest(part1, part2, part3)
                    : new HttpRequestBase(part1, part2, part3);

InTriggerMessageReceived:

var request = message as HttpRequest;
if (_messageSerializer != null && request != null)
{

Shouldn't if (_messageSerializer != null) { ... } be enough here?

crisim commented 9 years ago

HttpRequest has the cookie collection (which i think should be in the base class), the form collection and the file collection. These collections are populated by the _messageSerializer, so if you don't specify a message DEserializer then you shouldn't need them, maybe 'cause you deserialise the body and headers in your own way (implementing a SOAP Web Service?).

HttpRequestBase.CreateResponse create a HttpResponseBase which doesn't have the cookie collection, HttpRequest.CreateResponse create a HttpResponse which has the cookie collection.

AFAIK this is the only reason, maybe the author can explain us....

jgauffin commented 9 years ago

The purpose is to be able to pass the body just as it is. for instance if you want to build an application server or a proxy it wouldn't be interested in wasting memory on deserializing data when the form will not be used.

At least that's the purpose, maybe I didn't implement that all the way. Please take another look since I've given the HTTP parts a bit more love.

wo80 commented 9 years ago

Yeah, but I don't see why this should affect what type to return. It's the HttpMessageDecoder that decides if the body should be parsed or not ... just return HttpRequest in any case.

BTW: this is all the cookies fault ;-) When I had a first look at this project, I was casting an IClientContext..RequestMessage to IHttpRequest - but it has no cookie collection. Then I was casting to HttpRequest, but was getting a null reference exception (because I hadn't added a body decoder). I then found it was the HttpMessageDecoder causing the trouble and wondered why it was returning different types ...

jgauffin commented 8 years ago

You are of course correct. Design for the most common case. I've removed the base classes and updated the interfaces. How it doesn't cause too much trouble.