xyncro / freya

Freya Web Stack - Meta-Package
https://freya.io
Other
329 stars 30 forks source link

more OPTIONS issues, this time with etags #186

Closed Vidarls closed 8 years ago

Vidarls commented 8 years ago

there seems to be no way to avoid executing the logic that generates an e-tag when server receives an OPTIONS request. In my case this means that the db query gets executed twice for any CORS request :cry:

I'm not sure that responses to OPTIONS should have etags, can't seem to find the definitive answer, but there is no easy way of making this conditional. Currently I'm handling this by replying with a newguid eTag to OPTIONS requests, but I would rather just not include an e-tag with the OPTIONS reply.

kolektiv commented 8 years ago

This is odd, from my memory of the processing graph in Freya until now, OPTIONS requests should be doing any of the negotiation side of things at all. I wonder if it's simply a question of the handler doing too much. I'll check.

kolektiv commented 8 years ago

Right, yes, the Freya OPTIONS operation invokes etag, last modified, etc. (https://github.com/freya-fs/freya/blob/master/src/Freya.Machine.Extensions.Http/Operations.fs#L169-L175) and I don't think it should. OPTIONS requests aren't cacheable, and they shouldn't be treated as resources. I think that everything but the status and phrase could probably be removed from that, which would solve your problem. I'll have a chance to modify that over the weekend, although if you have a moment to do it and send a P making just that change, I could probably merge that on sight! If not, I'll do it ASAP.

(I'm trying to think if there would have been an original reason to include that - I think it may just have been a faulty assumption around being the same as OK, in which case it should simply go). Now that I'm looking at it, it doesn't seem to be fulfilling the other thing I'm sure it did, in terms of available methods, etc. (Allow header, etc.) which I may change at the same time. It's implemented for 405 responses, so should be a minor addition.

kolektiv commented 8 years ago

Have just merged #187 which should hopefully change the operation for OPTIONS and stop it setting headers that aren't properly needed. Let me know if this improves things!

Vidarls commented 8 years ago

:+1: Will take it for a spin next week

Vidarls commented 8 years ago

Etag is no longer called on options requests as far as I can see :+1: but now I have a question about the Freya.memo, as my query is again running twice on each request.:


let myData param andafunc = 
    freya {
        let! param = param
        return andafunc param
     } |> Freya.memo

If I call the above func twice in the same request with the same params, should it not execute only once?

kolektiv commented 8 years ago

Hmmm. The memoization I think is only likely to work as intended for declarations (parameter free) rather than functions. As it stands, every time you call that function it'll generate a new memoized function rather than simply evaluate the function. That might sound quite useless, but sometimes it is possible to make it so that chains of memoized functions are useful (for example, if those params were constructed from e.g. parts of the route path). Where are your params coming from? That'll determine whether simple memoization will work.

It would potentially be possible to construct a memoization function which could memoize functions taking parameters, and use the parameter values as part of a memoization key.

Hopefully that makes sense, let me know if it doesn't :smile:

Vidarls commented 8 years ago

Ok, I kind of figured that would be the case. I can work around it by using some composition patterns I believe. At least now we can close this issue.

kolektiv commented 8 years ago

Great on the original issue! I'll have a think about whether more advanced memoization is worth some implementation effort...