Skip to content
This repository has been archived by the owner on Oct 18, 2018. It is now read-only.

Reduce spread of [GeneralWebHook] support #258

Closed
dougbu opened this issue Mar 7, 2018 · 9 comments
Closed

Reduce spread of [GeneralWebHook] support #258

dougbu opened this issue Mar 7, 2018 · 9 comments
Assignees
Labels
3 - Done cost: S Will take up to 2 days to complete enhancement Mapped to BigRock This issue is mapped to a big rock

Comments

@dougbu
Copy link
Member

dougbu commented Mar 7, 2018

Most of the WebHook constraints and filters do runtime look-ups (Linq queries) to find their metadata. This is decentralized even though it's not required for anything but actions with [GeneralWebHook] applied. Thought is to instead apply specific constraints and filters when processing the application model.

For the [GeneralWebHook] case, use different constraint and filter constructors that pass a new interface. That interface would hide the runtime lookup behind its facade. When processing a request, constraints and filters would use the interface to grab the metadata they need, then proceed in the normal (receiver-specific) way.

Should not repeat any requirements the application model code enforces in the code for the [GeneralWebHook] case. Instead assume the retrieved metadata is consistent and let the constraints and filters "blow up" if not. This avoids additional code duplication and complexity.

@dougbu
Copy link
Member Author

dougbu commented Mar 7, 2018

Self-assigning because this needs to be done in the Preview2 milestone. Will otherwise be locked into the current approach.

@dougbu dougbu added this to the 1.0.0-preview2 milestone Mar 7, 2018
@dougbu dougbu self-assigned this Mar 7, 2018
@dougbu dougbu added enhancement cost: M Will take from 3 - 5 days to complete Mapped to BigRock This issue is mapped to a big rock labels Mar 7, 2018
@dougbu
Copy link
Member Author

dougbu commented Mar 7, 2018

@mkArtakMSFT doing this may reduce the size of other tasks and issues in this repo a bit. Issue is itself on the S / M border because it should mostly be mechanical.

@dougbu
Copy link
Member Author

dougbu commented Mar 7, 2018

BTW this is tied to main ASP.NET Core WebHooks 1.0.0 rock because [GeneralWebHook] actions are central to that.

@dougbu
Copy link
Member Author

dougbu commented Mar 9, 2018

/fyi @rynowak

dougbu added a commit that referenced this issue Mar 30, 2018
- #258
- centralize and minimize metadata lookups, especially per-request lookups
- add `abstract` `WebHookReceiverMetadataProvider` and implementation (`DefaultWebHookMetadataProvider`)
- add core constraints and filters to actions from `IApplicationModelProvider` implementations, not globally
  - these use `WebHookReceiverMetadataProvider` in `[GeneralWebHook]` scenarios
- add receiver-specific filters to actions from `WebHookActionModelFilterProvider` or `WebHookReceiverFilterProvider`
  - pattern is somewhat simpler than that for core filters
    - receiver-specific filters _never_ need to check their own applicability
  - add an `IFilterProvider` implementation: `WebHookReceiverFilterProvider`
  - add `IWebHookFilterMetadata` to ease registration of receiver-specific filters
- consolidate and rename lots of classes
  - give `IApplicationModelProvider` implementations clearer, more specific names

nits:
- shorten a few long lines
- Vs removed some BOMs

WIP
- finish remaining filters
- instantiate all core filters (not just `WebHookVerifyBodyTypeFilter`) in `WebHookActionModelFilterProvider`
- add more metadata to `ActionModel.Properties` in `WebHookActionModelPropertyProvider`
- fix tests
@dougbu dougbu modified the milestones: 1.0.0-preview2, 1.0.0-rc1 Mar 30, 2018
@WestDiscGolf
Copy link

Any update on the above and associated pull request? is this the last large change which will be going in before the full aspnet core 2.1 release?

Cheers

@dougbu
Copy link
Member Author

dougbu commented Apr 9, 2018

Any update on the above and associated pull request?

Have been doing other things but will get back to this soon.

is this the last large change which will be going in before the full aspnet core 2.1 release?

Not necessarily. Why do you ask?

@WestDiscGolf
Copy link

Thanks for the update. I've been working on a Webhook that I want to nuget up but have been developing it in this solution to aid with debugging and getting the latest changes as the library has been developing. Just curious how much I'd have to tweak before I can get a preview out based on a stable nuget :-)

dougbu added a commit that referenced this issue Apr 9, 2018
- #258
- centralize and minimize metadata lookups, especially per-request lookups
- add `abstract` `WebHookMetadataProvider` and implementation (`DefaultWebHookMetadataProvider`)
- add core constraints and filters to actions from `IApplicationModelProvider` implementations, not globally
  - constraints and filters use `WebHookMetadataProvider` in `[GeneralWebHook]` scenarios
- add receiver-specific filters to actions from `WebHookActionModelFilterProvider` or `WebHookFilterProvider`
  - pattern is somewhat simpler than that for core filters; these filters need to check their own applicability
  - add `WebHookFilterProvider`, an `IFilterProvider` implementation
  - add `IWebHookFilterMetadata` to ease registration of receiver-specific filters
- implement `IOrderedFilter` in all filters
- consolidate and rename lots of classes
  - give `IApplicationModelProvider` implementations clearer, more specific names

nits:
- make doc comments more consistent
- correct a few oddly-named fields and variables
- shorten a few long lines
@dougbu dougbu added cost: S Will take up to 2 days to complete and removed cost: M Will take from 3 - 5 days to complete labels Apr 10, 2018
@dougbu
Copy link
Member Author

dougbu commented Apr 10, 2018

Reduced cost to track the review process that remains in this milestone.

@dougbu
Copy link
Member Author

dougbu commented Apr 11, 2018

c9ab0b4

@dougbu dougbu closed this as completed Apr 11, 2018
dougbu added a commit that referenced this issue Apr 13, 2018
- register `DefaultWebHookMetadataProvider` in DI
- correct names of resources
dougbu added a commit that referenced this issue Apr 13, 2018
- register `DefaultWebHookMetadataProvider` in DI
- correct names of resources
dougbu added a commit that referenced this issue Apr 16, 2018
- register `DefaultWebHookMetadataProvider` in DI
- correct names of resources
dougbu added a commit that referenced this issue Apr 17, 2018
- #275
- register `DefaultWebHookMetadataProvider` in DI
- register `IWebHookFilterMetadata` implementations in DI
- fix parameter reversal and overuse of `Resources.VerifyMethod_BadMethod`
- add first functional tests
  - part of #180
  - add functional test infrastructure
  - update `DropboxCoreReceiver` to meet new requirements e.g. add test-only secret key

nits:
- correct names of resources
- let VS remove a few UTF8 BOMs
WestDiscGolf pushed a commit to WestDiscGolf/WebHooks that referenced this issue Apr 17, 2018
- register `DefaultWebHookMetadataProvider` in DI
- correct names of resources
dougbu added a commit that referenced this issue Apr 17, 2018
Merge to `dev`: Fix gaps in final commit for #258 (c9ab0b4)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - Done cost: S Will take up to 2 days to complete enhancement Mapped to BigRock This issue is mapped to a big rock
Projects
None yet
Development

No branches or pull requests

2 participants