Skip to content

Simplified signature of app.UseJsonApi() #750

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

Merged
merged 2 commits into from
May 22, 2020
Merged

Simplified signature of app.UseJsonApi() #750

merged 2 commits into from
May 22, 2020

Conversation

bart-degreed
Copy link
Contributor

Fixes #733.

@bart-degreed bart-degreed requested a review from maurei May 13, 2020 10:55
@maurei
Copy link
Member

maurei commented May 15, 2020

Thanks for this work!

Its a good thing to have full control over the registration of everything without having to worry about callig internal services like IInverseRelationshipResolver. Right now UseJsonApi() only calls builder.UseMiddleware<JsonApiMiddleware>();. Perhaps worth considering to just have the user call the latter directly? This would be in line wit having total control over the middleware pipeline

We could then keep UseJsonApi() as a shortcut for the obvious boilerplate usecases in which there is less control over what is registered and what is not. I like having boilerplate methods like these that obfuscate internals (eg that JsonApiMiddleware must be registered BEFORE endpoint registration but AFTER UseRouting). We could add a few more that give control over authentication:
- UseJsonApiWithoutAuthorization() (no authentication or authorization)
- UseJsonApiWithAuthentication() (no authentication)
- UseJsonApi() (both authentication and authorization)

@bart-degreed
Copy link
Contributor Author

To me it seems there's a common pattern in ASP.NET Core where middleware provides app.UseXXX() as a shortcut to calling .UseMiddleware<>. Sticking with that makes our approach intuitive. It also enables us to do internal refactoring without affecting the public signatures (for example, if we find that a specific setting is not compatible in the future, we can detect and throw from here).

Authorization/Authentication are just two examples of widely-used middleware. There's a lot more out there, so I wouldn't feel comfortable trying to predict the users' needs by dictating how ordering must be. Also the order has some rules, but there is no strict ordering that is only correct, it depends on context. For example, a dev may put an exception filter at the top of the middleware, while someone else put that lower, to increase perf on serving static files.

The pipeline ordering is an important ASP.NET Core concept that is widely documented externally. We should provide our needs, using examples, but also provide the freedom to order as you see fit. I still think that is the best we can do: guide in the right direction, but at the same time be open if other orderings are needed.

@bart-degreed bart-degreed merged commit 4f6b1cd into json-api-dotnet:master May 22, 2020
@bart-degreed bart-degreed deleted the use-jsonapi branch May 22, 2020 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup implementation of UseJsonApi
2 participants