Handling fetchModel errors and Update being polymorphic over PUT, PATCH and POST requests

[NinoFloris]

Some of the issues I've already encountered and why it shouldn't be two separate issues ;)

1. Not possible to use different models/cases for POST, PUT and PATCH (we use json patch for PATCH requests).
2. Hard to process parse (json.net) errors with what's in the box as exceptions are swallowed.
3. Null isn't dealt with correctly, no check before wrapping deserialize result in Some, yay Some(null).
4. Developer error, not plugging fetchModel before calling loadModel or fetchModel threw exception is why you'll get a None from loadModel right now, first should be an exception, second should be an error you can disambiguate from the first case and deal with.

Also until https://github.com/giraffe-fsharp/Giraffe/issues/279 is fixed BindModelAsync isn't actually binding the body for PATCH requests, absolutely breaking any Saturn Update handler that uses both PUT and PATCH right now ^_^

Patches I currently use per issue

1. Custom fetchModel that slams PUT and PATCH into a union JsonUpdate<'a> and works around giraffe issue, a generic fetchUpdateModel that can deal with other unions would be nice but is unlikely as cases aren't types .
2. Directly handle deserialize exceptions in custom fetchModel which has an exception handler argument.
3. Null is fixed with Option.ofObj before returning RequestModel inside loadModel
4. Disambiguating is ish fixed by these other patches.

Actual improvements I'd like to see

• A way to subdivide update between put and patch so no need for the union, also helps with closing down patch when you only want put.

I started with matching inside handler but then error response order is wrong (e.g. first you get fetchModel parse error response, you fix that and only then get method not allowed response) so now I do the blocking inside an Update plug where it returns method not allowed and stops. Not ideal at all.

could have been be 'cleanly' fixed by custom operation overload ... where second overload accepts patchhandler option and if None it returns method not allowed.

If the answer is just to use a wrapper handler accepting handlers for POST/PUT/PATCH then why combine it in the first place anyway.

• Many fixes to fetchModel and loadModel so they can deal with all types of errors that are happening.

Honestly I would want RequestModel to always be unset or either contain an Option<'Model>, this would help a lot with disambiguation and null value handling.

PR time?

P.S. not mentioned above

We currently use extra custom ops on controller for show/create/etc that pushes in the model as an arg, called like updateModel etc. I would hope custom operation overload could help a bit here as well in the future like with https://github.com/SaturnFramework/Saturn/issues/37

[rmunn]

could have been be 'cleanly' fixed by custom operation overload ... where second overload accepts patchhandler option and if None it returns method not allowed.

I like this idea; once RFC FS-1056 goes in, this would definitely be a good idea to add to Saturn.

[Krzysztof-Cieslak]

Not possible to use different models/cases for POST, PUT and PATCH (we use json patch for PATCH requests).

This is somehow a by design behavior in controller CE, if you need more control over what's going on you can fallback to lower level... however we can discuss it, i can see how having exactly same handler for all those 3 cases may be problematic.

Also, I think there is one super smart workaround... you can use plug on controller level just for PATCH action to propagate model differently than in case of POST. And then just work on this model in handler.

Many fixes to fetchModel and loadModel so they can deal with all types of errors that are happening.

Please send PRs ;)

Hard to process parse (json.net) errors with what's in the box as exceptions are swallowed.

Hmmm. Yes, i can see that being an issue

I like this idea; once RFC FS-1056 goes in, this would definitely be a good idea to add to Saturn.

I can't wait for custom operation overloading, it will allow some really nice APIs to be included in Saturn.

[NinoFloris]

Also, I think there is one super smart workaround... you can use plug on controller level just for PATCH action to propagate model differently than in case of POST. And then just work on this model in handler.

This would be really nice, however as fetch and load are split you actually would need to special case fetchModel to store a different model. And if you would do that, without special casing on request method inside your controller handler you wouldn't know the type to retrieve from loadModel, either blowing up because of an invalid cast or retrieving the wrong model :/ So there's nothing nicely generic to be made here, mostly because you lose your type info after the fetch, I'll try to find the best compromise :)

About the Update handling POST PUT PATCH, what might be a decent compromise is to have some custom operations patchUpdate/putUpdate/postUpdate as type extensions in a different module for people to easily split update into individual handlers.

So something that would work like this, with handler order matching declaration order inside your controller.

member __.PostUpdate state handler =  
    {state with Update = choose [ POST >=> postUpdate; state.Update] }

Having all 3 (4 wouldn't make sense) in use would then bring some significant nested probing to find the right handler though.

Edit: these three could also be one and always included, something like updateSplit (post option) (put option) (patch option) but it's layering less nicely. E.g. No real mix and match as with the 3 custom ones you could have in use both patchUpdate and then update for the rest.

[NinoFloris]

Now that I think about this again, it's not amazing because you cannot make new plug cases :/ so you would have to manually choose at the plug level too...

[Krzysztof-Cieslak]

What about adding keywords that will enable you to override PATCH and PUT? So for patch it would be if state.Patch.isSome then state.Patch elif state.Update.isSome then state.Update else ...

[NinoFloris]

So I really don't like the overriding aspect as things stay open that way, your update handler might really not expect patch or post and just do something that isn't in the spirit of either request method (for instance calling patch which then hoses your document to nulls/Nones because you just built for put). That might even be a stronger argument for going with one extra keyword that just splits it out.

I have a little snippet by now that I'm going to PR with a bunch of other things

[Krzysztof-Cieslak]

So after discussion with @NinoFloris on Slack:

1. Be more clear in docs that there exists POST at / which is Create method
2. Be more clear in docs that POST at /:id is Update
3. Keep POST and PUT at /:id as Update action
4. Separate PATCH at /:id as Patch action with new keyword and plug

[rmunn]

For reference, https://github.com/giraffe-fsharp/Giraffe/issues/279 is fixed in Giraffe's develop branch now, so it'll probably make it into Giraffe's 1.2 release whenever that ends up happening.

[Krzysztof-Cieslak]

90 split Patch and Update, #102 fixed the fetchModel and loadModel