Issue with "resource/" and "resource" routing, triggering twice

[NinoFloris]

Best seen with having a create and update handler, try to POST to "/resource/" two plugs will fire, first for create then for update.

This is baddd...

[talbottmike]

@NinoFloris I'd like to contribute by adding some tests. Maybe, as a step towards solving, I could add a test to reproduce this issue. Is this bug specifically related to controllers? Do you have an example somewhere that I could review?

[jeremyabbott]

@NinoFloris the controller unit tests include tests for create and update. In the tests , the create action is only called once, and the update action is only called once.

I created a test project with the following controller:

type Foo = { Name: string }


let api = controller {
    create (fun ctx -> ctx.GetLogger().LogInformation("create"); Controller.json ctx  {Name = "foo" })
    update (fun ctx name -> ctx.GetLogger().LogInformation("update"); Controller.json ctx {Name = name} )
}

And I do not see "update" logged when I hit the POST endpoint.

[NinoFloris]

It's about the plugs

[StachuDotNet]

@NinoFloris Can you please provide more information as to what is broken?

A broken code example would be lovely.

[jeremyabbott]

@StachuDotNet I was able to reproduce the issue locally, and have a unit test that causes the issue. I have a tentative fix. I’ll push up the failing test this evening.

[NinoFloris]

Thanks @jeremyabbott. Sorry I'm currently swamped, a repro could have helped

[jeremyabbott]

@StachuDotNet here's my branch with a failing test that indicates what's happening.

Here's a description of what's happening from the commit I made:

What's happening?

1. AddHandler (or AddKeyHandler) is called for each action. The AddHandler method composes the specified plugs with each HTTP verb.
2. The Update action gets wired up with plugs for POSTs to resource/id and for PUTs
3. The Create action gets wired up with plugs for POSTs to resource

if you look at lines 234/235 we add plugs for create for POST. This is fine

We then do this again for at line 241 for Update when Update.IsSome (line 238). NOW we have the plug applied for POST twice.

Ideally we should only apply the plug to a verb once. Talking to Krzystof last week, he was hesistant about changing the methods (or adding separate methods for applying the plugs). We could keep track of whether or not a plug has been applied for a method though, and not apply it if it's already been applied. Since the entire handler is composed with a verb at the same time the plugs are applied, we would need to associate the plugs with an action, and then associate the action with the handler, and only associate the plug with the action once. This may mean keeping track of separate handlers for the verb portion, applying the plugs, and then composing the action/handler with the underlying handler associated with that action.

I'm planning on looking at it some more tomorrow night (or later this evening...)

[jeremyabbott]

@StachuDotNet actually I think this happens without update. So my explanation above is wrong. If you look at this commit, you'll see where I've commented out the update tests, and the create test still fails. We add the handlers twice for Create at lines 234/235 of Controller.fs (my branch). The plug is applied to each handler, so my best guess is that each handler is getting evaluated, but only the second handler is executed. When I comment out 234, the test passes again.

[jeremyabbott]

It does happen if you make a POST and you have and Update action defined. This is because of line 234 as well. If we make a POST to resource/1 we we hit the plug added by 234 for [Create] because we haven't added the route until after the plug is applied. I think the intent should be for routes/verb pairs should be synonymous with the corresponding action name. So create = POST "/" (or ""), and update = POST "/{id}" or PUT "/{id}. This will happen in any scenario where we add two handlers with different paths for the same action.

[jeremyabbott]

@NinoFloris @StachuDotNet I think I figured it out. PR here which prevents plugs from firing twice, and also tests controllers to make sure they continue to work as expected.

[Krzysztof-Cieslak]

Fixed by #143