Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lava Webhook of Method PUT or PATCH don't resolve #3277

Closed
1 task done
tscrip opened this issue Sep 14, 2018 · 6 comments
Closed
1 task done

Lava Webhook of Method PUT or PATCH don't resolve #3277

tscrip opened this issue Sep 14, 2018 · 6 comments
Assignees
Labels
Fixed in v17.0 Priority: Low Affects a small number of Rock installations and will not be noticed by most users. Status: Confirmed It's clear what the subject of the issue is about, and what the resolution should be. Type: Bug Confirmed bugs or reports that are very likely to be bugs.

Comments

@tscrip
Copy link

tscrip commented Sep 14, 2018

Prerequisites

Description

If you create a lava webhook with a method of PUT or PATCH, it does not seem to add the route. If you switch it to POST, it works fine.

Steps to Reproduce

  1. Create a Lava Webhook called /test123
  2. Set the method to PUT
  3. Using Postman, hit the URL with the PUT Verb (you should get "this page does not exist")
  4. Update your webhook to PATCH
  5. Using Postman, hit the URL with the PATCH Verb (you should get "this page does not exist")
  6. Update your webhook to POST
  7. Using Postman, hit the URL with the POST Verb (Success!)

Expected behavior:
PATCH and PUT routes should be defined.

Actual behavior:
Rock doesn't know they exist.

Versions

  • Rock Version: 7.5
  • Client Culture Setting: en-US
@cabal95
Copy link
Member

cabal95 commented Sep 14, 2018

Admittedly, I never tested this with PUT and PATCH specifically. I just included those verbs as a "why not". I wonder if IIS is internally trying to handle those rather than letting the web hook handle it.

@jonedmiston
Copy link
Member

@cabal95 are you able to look at this?

@jonedmiston jonedmiston added the Status: Available The core team has made this available for the community to fix. (was: Status: Available) label Sep 14, 2018
@cabal95
Copy link
Member

cabal95 commented Sep 18, 2018

Yes, I'll take a look and see if I can verify what the core issue is.

@cabal95
Copy link
Member

cabal95 commented Sep 18, 2018

It looks like this is an issue at the web.config level. Or more specifically, at the "server" level web.config.

The default handler mapping for .ashx files appears to only support GET, HEAD, POST, and DEBUG (oddly enough). I tested enabling the other two on my IIS Express and it seems to work. If you want to test enabling this in full IIS and report back we can discuss if it is something we should:

  1. Override in web.config automatically via an update
  2. Update the Defined Type to include instructions on how to do this manually if you want to use PUT, PATCH or DELETE
  3. Just remove those three options. (I'd like to keep them, but I'm not married to it since I don't use those 2 verbs myself)

To enable for testing:

  1. Open IIS Manager and go to your Site.
  2. Go to the Handler Mappings
  3. Look for the 6 *.ashx handlers (pictured below).
  4. Open each one in turn, click Request Restrictions, go to the Verbs tab, and add ,PUT,PATCH,DELETE to the text box
  5. (Doing this will almost certainly restart Rock, so make sure you don't do it during prime time)

edit: picture I forgot:

image

@jonedmiston
Copy link
Member

Is not having support for PUT/PATCH a deal since Lava can't update data?

@cabal95
Copy link
Member

cabal95 commented Sep 18, 2018

It's not a huge issue, but the Lava Webhooks have their own Entity Command enabling, so they can use {% sql %} statements to make updates. We are actually doing this with a metric via a GET request to a specific API (probably should have made it a POST or PUT or something but wasn't thinking that far ahead). Every hit to the API runs a SQL query that increments the value of a specific metric.

A fourth option, would be to convert the attribute to be a plain Text attribute. This shouldn't cause any backwards compatibility issues. It would require the user to specifically put in the verb name (GET, POST, etc.) - though we could make it default to GET if not specified as that is probably the most common one. This would allow for people to still use the "unsupported" verbs if they want without advertising that they should work out of the box.

@cabal95 cabal95 added Type: Bug Confirmed bugs or reports that are very likely to be bugs. Status: Confirmed It's clear what the subject of the issue is about, and what the resolution should be. Priority: Low Affects a small number of Rock installations and will not be noticed by most users. labels Jan 4, 2019
@nairdo nairdo added Solution Required Items that need a designed solution before they can be marked Available and removed Status: Available The core team has made this available for the community to fix. (was: Status: Available) labels Oct 21, 2019
@cabal95 cabal95 added Fixed in v17.0 and removed Solution Required Items that need a designed solution before they can be marked Available labels Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in v17.0 Priority: Low Affects a small number of Rock installations and will not be noticed by most users. Status: Confirmed It's clear what the subject of the issue is about, and what the resolution should be. Type: Bug Confirmed bugs or reports that are very likely to be bugs.
Projects
None yet
Development

No branches or pull requests

5 participants