Skip to content
This repository has been archived by the owner. It is now read-only.

Start of an Asp.Net Core port #47

Closed
wants to merge 15 commits into from
Closed

Conversation

@rposener
Copy link
Contributor

@rposener rposener commented Mar 6, 2016

Haven't had a lot of time to do much else, yet. Coming weeks should provide more options. Any chance you guys will be at Build event? Would like to see a V1 of this published as nupkg's by then. If you guys can provide some guidance / how IServiceCollection should/should not be used, etc. that would be helpful. Also, should it leverage the HttpRequest ApplicationServices property to do all resolution? Do we need the Default Service functionality, or should that be handled as a default during registration using an option lambda on the extension method for Configuring WebHooks?

/// Extension methods for <see cref="IDependencyScope"/> facilitating getting the services used by custom WebHooks.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public static class DependencyScopeExtensions

This comment has been minimized.

@davidfowl

davidfowl Mar 6, 2016
Member

Too much service locator. Can these be properly injected?

/// Extension methods for <see cref="string"/>.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public static class StringExtensions

This comment has been minimized.

@davidfowl

davidfowl Mar 6, 2016
Member

Move these to a "Internal" namespace. Extension methods on string are generally a bad idea.

}

services.AddSingleton<DefaultAssemblyProvider>();
services.Configure<SettingsDictionary>(opt =>

This comment has been minimized.

@davidfowl

davidfowl Mar 6, 2016
Member

SettingsDictionary seems like an odd thing to configure. Why not WebHookOptions?

/// Extension methods for <see cref="IDependencyScope"/> facilitating getting the services used by custom WebHooks.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public static class DependencyScopeExtensions

This comment has been minimized.

@davidfowl

davidfowl Mar 6, 2016
Member

See earlier comment.

@rposener
Copy link
Contributor Author

@rposener rposener commented Mar 8, 2016

Does this look more in line with the approach you would expect?

@rposener
Copy link
Contributor Author

@rposener rposener commented Mar 28, 2016

Just a note about couple items that need input / review:

  1. IOptions/IConfiguration setup for secrets / settings
  2. CustomWebHookReceiver can't seem to read request body (it's all default initialized bytes of 0) not sure what I've missed on that?
  3. Configuration Extensions of Services / Resolution of Services may need some revision - was not sure how to setup configuration extensions with options Action to pass in configuration
  4. Error Handling for various accept types in the MiddleWare (I changed all exceptions to use a custom Exception type to specify status code + message) since HttpResponseException is not part of ASP.Net Core.
@Eilon Eilon self-assigned this Mar 28, 2016
@@ -57,12 +57,14 @@ public static void ConfigureMapping(IApplicationBuilder app)
/// <returns></returns>
private static async Task ProcessWebHook(HttpContext context, string webHookReceiver, string id)

This comment has been minimized.

@davidfowl

davidfowl Mar 31, 2016
Member

I would change this into a proper middleware class so that dependencies can be injected once instead of being pulled out of the container manually.

This comment has been minimized.

@rposener

rposener Mar 31, 2016
Author Contributor

Question: Should I revise the entire thing to merge the managers into the middleware, Simplifying the entire model? Basically have Middleware > Receivers > Handlers? This feels like a much simpler approach and having been through enough now, this may make for a simpler solution.

@rposener
Copy link
Contributor Author

@rposener rposener commented Apr 20, 2016

Any further information / details regarding this? Does it need to get updated to the current dev branch / RC2? Is it a good time to do that, or should we hold off until it releases?

@HenrikFrystykNielsen
Copy link
Contributor

@HenrikFrystykNielsen HenrikFrystykNielsen commented Apr 20, 2016

I am sorry it takes so long. I have poked the 'core' folks but I think they are busy. Will try again.

.gitignore Outdated
@@ -18,3 +18,5 @@ _[Ss]cripts
*.dot[Cc]over
*.pubxml
*.zip
/coresamples/CoreCustomSender/project.lock.json

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Should just exclude all project.lock.json files - no need to check them in for this repo.

</div>

<environment names="Development">
<script src="~/lib/jquery/dist/jquery.js"></script>

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

I recommend removing jQuery and all other libraries that are available on CDNs, and instead reference the files directly from a CDN. This avoid having ~50,000 lines of JS files checked into the repo.

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

You can find CDN info here: http://www.asp.net/ajax/cdn

global.json Outdated
@@ -0,0 +1,6 @@
{
"projects": [ "src", "." ],

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Does the . folder need to be listed? All the source projects are in the src folder as far as I can tell.

@Eilon
Copy link
Member

@Eilon Eilon commented May 17, 2016

{

This file shouldn't be needed; the root folder's global.json should be enough.


Refers to: src/Microsoft.AspNetCore.WebHooks.Common/global.json:1 in 1b1edb7. [](commit_id = 1b1edb7, deletion_comment = False)

/// Extension methods for <see cref="IDictionary{TKey,TValue}"/>.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public static class DictionaryExtensions

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

All these extension methods types need to be internal. We don't want them "leaking out" of this assembly.

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Used anywhere?

/// or <c>text/xyz+xml</c>. The term <c>xyz</c> can for example be <c>rdf</c> or some other
/// XML derived media type.
/// </summary>
/// <returns>true if the specified content is JSON content; otherwise, false.</returns>

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Doc comment is wrong 😄

/// XML derived media type.
/// </summary>
/// <returns>true if the specified content is JSON content; otherwise, false.</returns>
/// <param name="content">The content to check.</param>

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

No such param.

/// JSON derived media type.
/// </summary>
/// <returns>true if the specified content is JSON content; otherwise, false.</returns>
/// <param name="content">The content to check.</param>

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

No such param.


namespace Microsoft.AspNet.WebHooks
{
public class DefaultAssemblyProvider

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

BTW starting in RC2 there's a new feature called Application Parts that will make this code much nicer! Will be interesting to see what can be simplified when this is updated to RC2.

/// <param name="user">The <see cref="IWebHookUser"/> for the current user</param>
/// <param name="loggerFactory">the default <see cref="ILoggerFactory"/> logger factory</param>
public WebHookRegistrationsController(IWebHookManager manager,
IWebHookStore store, IWebHookUser user, ILoggerFactory loggerFactory)

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

BTW you can just take in ILogger<WebHookRegistrationsController> and the DI system will provide the correct instance.

return Ok();

case StoreResult.Conflict:
return new HttpStatusCodeResult((int)System.Net.HttpStatusCode.Conflict);

This comment has been minimized.

/// Provides basic data model validation based on data annotations.
/// </summary>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
internal sealed class ValidateModelAttribute : ActionFilterAttribute

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

I don't think I saw this used anywhere...?

@Eilon
Copy link
Member

@Eilon Eilon commented May 17, 2016

Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Src.AspNetCore", "Src.AspNetCore", "{668B3959-3928-472F-9341-06BF8C9BADE9}"

I might just be misreading the SLN file, but are the names and paths of the projects here correct?


Refers to: WebHooks.sln:145 in 1b1edb7. [](commit_id = 1b1edb7, deletion_comment = False)

public class CustomFilterProvider : IWebHookFilterProvider
{
private readonly Collection<WebHookFilter> filters = new Collection<WebHookFilter>
{

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Formatting got messed up here.

@@ -0,0 +1,53 @@
{

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Maybe I'm not seeing it in the commit, but where's the Program.cs or Startup.cs of this sample?

@@ -0,0 +1,53 @@
{

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

BTW if this app doesn't need users and membership, could start out with a much simpler template.

[HttpPost]
public async Task<IActionResult> Post([FromBody]string value)
{
await this.NotifyAsync("event2", new { P1 = "p1" });

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Ok I don't know how I'm missing this, but where is NotifyAsync defined??

/// <summary>
/// Utilities for converting to and from hex-encoded and base64-encoded strings.
/// </summary>
public static class EncodingUtilities

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Used anywhere?

/// <summary>
/// Provides non-cryptographic hashing functions.
/// </summary>
public static class Hasher

This comment has been minimized.

@Eilon

Eilon May 17, 2016
Member

Used anywhere?

Added WebHookNotification Classes
@rposener
Copy link
Contributor Author

@rposener rposener commented May 23, 2016

OK I've updated my fork with a bunch of new projects for the core stuff. I have a few questions that I need some input on.

  • For Receivers Middleware now calls Receivers, which produce a Context if the Handlers should be called. If no handler should be called (e.g. echo/Validation) then it just returns null. Otherwise, they set the context, or in error situation throw.
  • All options are set via IOptions, and ServiceCollectionExtensions and MiddlewareExtensions have a setupAction to apply conifguration options as necessary.
  • NotificationDictionary is now IWebHookNotification (I created 2 classes to support existing items, but should provide nice options for DTO's for WebHooks.
  • Resources are not loading - trying to test sending of custom notification seems to die. Not sure what/how I've messed up resources. Any information anyone can provide would be helpful.
  • Overall it converted much easier on RC2, some is just my experience with asp.net core growing, some is the tooling!
  • Items not necessary for asp.net core have been omitted. Projects should be setup a bit cleaner now.
@rposener
Copy link
Contributor Author

@rposener rposener commented May 24, 2016

Figured out my Resource Loading issue (https://github.com/dotnet/corefx/issues/8754)

rposener added 2 commits May 24, 2016
Registrars as Optional (Resolve on Demand)
Custom Subscribe/Unsubscribe / Notification working
@rposener
Copy link
Contributor Author

@rposener rposener commented May 24, 2016

Ok - custom senders and receivers are working with all the same interfaces implemented. I have not made my way back to writing tests, but generally all the same tests as the .net project should be passing.

Question for Henrik - I had not paid attention to the Registrars before. It doesn't look like any sample or other project implements them. Is that there just in case someone needs to do some additional verification? I also did not really get the point of the IdValidator along the same lines. Could those not be just consolidated into a single validator, and leave it up to the implementation to decide what to validate?

I am heading out on Vacation till next Tuesday. If you guys can give this a bit of a review (Eilon) in the mean time, I will start on converting the tests, and look at updating the other receivers, which should not take too much effort to crank through.

@MisterJames
Copy link

@MisterJames MisterJames commented Jun 21, 2016

@rposener @HenrikFrystykNielsen Hey folks, no milestone here, but it looks like it's getting close? Is there an ETA for this PR to get merged?

@rposener
Copy link
Contributor Author

@rposener rposener commented Jun 23, 2016

Well, I have had some time away where I was unable to work on it. Should be able to get more done soon. Kind of wish I could get some review from the asp.net core team to know if middleware is looking OK, and from Henrik based on questions above. Certainly is close - just want a little confidence I'm not severely overlooking something. If it's good, then we should be able to just start porting all the implementations and tests.

Or perhaps, the team wants it to be a single project that targets both frameworks. If that is the case, a lot of refactoring is probably needed... Open to input, and would love some feedback.

@rposener
Copy link
Contributor Author

@rposener rposener commented Jul 12, 2016

Should I just close this and let someone else start again?

@HenrikFrystykNielsen
Copy link
Contributor

@HenrikFrystykNielsen HenrikFrystykNielsen commented Jul 12, 2016

I really appreciate all the work you have put into it and I apologize for the lack of response. Part of it has been because core has been in flux but with core 1.0 out it should be possible to get a stable model together. I absolutely do want this to run on core but it has been difficult to get things to align. Let me chat with some of the core folks and see how we can bring your work forward. thanks!

@tutukar
Copy link

@tutukar tutukar commented Aug 15, 2016

Any word on if and when this branch is going to be merged to master? We have a Webhook based running we are looking to Migrate to .net core. Thanks!

@invisibleaxm
Copy link

@invisibleaxm invisibleaxm commented Sep 15, 2016

+1 it would be awesome if we can start developing webhooks in .net core

@dalestone
Copy link

@dalestone dalestone commented Sep 19, 2016

Any further update on this? It would be great if we could use WebHooks in .NET core

@binaryPUNCH
Copy link

@binaryPUNCH binaryPUNCH commented Sep 27, 2016

I'm super excited for this! Any eta?

@yilativ
Copy link

@yilativ yilativ commented Sep 30, 2016

Anyone try this yet?

@dkent600
Copy link

@dkent600 dkent600 commented Nov 7, 2016

It seems no one is paying attention

@CharlBest
Copy link

@CharlBest CharlBest commented Dec 9, 2016

This would be very cool indeed

@alhardy
Copy link

@alhardy alhardy commented Jan 10, 2017

We have a project were this would be very useful, hesitant to use it though because there doesn't appear to be much recent work done on the project, is work going to continue on the project?

@HenrikFrystykNielsen
Copy link
Contributor

@HenrikFrystykNielsen HenrikFrystykNielsen commented Jan 12, 2017

It is on the road map but the stars haven't aligned yet to make it come together. I will provide an update when that happens. Thanks! Henrik

@tutukar
Copy link

@tutukar tutukar commented Mar 3, 2017

Anything on this yet?

@henningst
Copy link

@henningst henningst commented Mar 20, 2017

When can we expect to get WebHooks support in ASP.NET Core? This blog post says:

A port to the ASP.NET Core is being planned so please stay tuned!

Could you share any more details on that?

@HenrikFrystykNielsen

@Eilon
Copy link
Member

@Eilon Eilon commented Oct 6, 2017

Closing this PR because we're going with #153 instead. Thanks!

@ghost
Copy link

@ghost ghost commented Oct 5, 2018

Webhook is not working for ASP.NET core 2.1 with GitHub. Does any one have any working samples.

@Eilon
Copy link
Member

@Eilon Eilon commented Oct 5, 2018

@ALMWEARE please log a new issue with details.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.