Skip to content

Add ConfigureRouteHandlerJsonOptions for M.A.Htt.Json.JsonOptions#39502

Merged
captainsafia merged 4 commits into
dotnet:mainfrom
captainsafia:safia/json-options
Jan 21, 2022
Merged

Add ConfigureRouteHandlerJsonOptions for M.A.Htt.Json.JsonOptions#39502
captainsafia merged 4 commits into
dotnet:mainfrom
captainsafia:safia/json-options

Conversation

@captainsafia
Copy link
Copy Markdown
Contributor

@captainsafia captainsafia commented Jan 13, 2022

Closes #39226.

@ghost ghost added the area-runtime label Jan 13, 2022
@captainsafia captainsafia marked this pull request as ready for review January 14, 2022 18:45
Tratcher
Tratcher approved these changes Jan 14, 2022
View reviewed changes
Comment thread src/Http/Http.Extensions/src/HttpJsonServiceExtensions.cs Outdated
BrennanConroy
BrennanConroy reviewed Jan 14, 2022
View reviewed changes
Comment thread src/Http/Http.Extensions/src/HttpJsonServiceExtensions.cs Outdated
pranavkm
pranavkm approved these changes Jan 14, 2022
View reviewed changes
Comment thread src/Http/Http.Extensions/src/HttpJsonServiceExtensions.cs
Comment thread src/Http/Http.Extensions/src/HttpJsonServiceExtensions.cs Outdated
/// <param name="configureOptions">The <see cref="Action{JsonOptions}"/> to configure the
/// <see cref="JsonOptions"/>.</param>
/// <returns>The modified <see cref="IServiceCollection"/>.</returns>
public static IServiceCollection ConfigureHttpJsonOptions(this IServiceCollection services, Action<JsonOptions> configureOptions)
Copy link
Copy Markdown
Contributor

@pranavkm pranavkm Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should review this name.

Copy link
Copy Markdown
Member

@halter73 halter73 Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the first time we're adding a ConfigureWhatever() IServiceCollection extension method for an option type? I guess in most cases it wouldn't be necessary because you could have an overload that takes a configureOptions callback to AddWhatever() for most option types.

The only thing I see right now that's close to this is ConfigureApplicationCookie() and ConfigureExternalCookie() which use named options.

Copy link
Copy Markdown
Contributor

@pranavkm pranavkm Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly the same, but we added ConfigureApiBehaviorOptions to MVC to solve a similar discoverability issue: https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.mvccoremvcbuilderextensions.configureapibehavioroptions?view=aspnetcore-6.0#microsoft-extensions-dependencyinjection-mvccoremvcbuilderextensions-configureapibehavioroptions(microsoft-extensions-dependencyinjection-imvcbuilder-system-action((microsoft-aspnetcore-mvc-apibehavioroptions)))

Copy link
Copy Markdown
Contributor Author

@captainsafia captainsafia Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the name to ConfigureRouteHandlerJsonOptions per our email conversation but open to other ideas.

@Kahbazi
Copy link
Copy Markdown
Member

Kahbazi commented Jan 15, 2022

Also Closes #35904

@halter73
Copy link
Copy Markdown
Member

halter73 commented Jan 18, 2022

@Kahbazi Do you not care about endpoint specific JSON options?

@Kahbazi
Copy link
Copy Markdown
Member

Kahbazi commented Jan 19, 2022

@Kahbazi Do you not care about endpoint specific JSON options?

@halter73 It's not an issue for me at the moment, since it's also doable by combination of BindAsync and Results.Json.
Also based on @davidfowl 's comment, I get the sense that that issue is limited to add a method to configure HttpJsonOptions.

halter73
halter73 reviewed Jan 20, 2022
View reviewed changes
Comment thread src/Http/Http.Extensions/src/HttpJsonServiceExtensions.cs Outdated
@captainsafia captainsafia changed the title Add ConfigureHttpJsonOptions for M.A.Htt.Json.JsonOptions Add ConfigureRouteHandlerJsonOptions for M.A.Htt.Json.JsonOptions Jan 21, 2022
@captainsafia captainsafia merged commit 7e6674c into dotnet:main Jan 21, 2022
@ghost ghost added this to the 7.0-preview1 milestone Jan 21, 2022
ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022
@ShreyasJejurkar
Add ConfigureRouteHandlerJsonOptions for M.A.Htt.Json.JsonOptions (do…
2c5b9cb 
…tnet#39502)

* Add ConfigureHttpJsonOptions for M.A.Htt.Json.JsonOptions

* Address initial feedback from API review

* Fix method reference in doc comment

* Fix up crefs
@captainsafia captainsafia deleted the safia/json-options branch February 4, 2022 22:50
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BrennanConroy BrennanConroy BrennanConroy left review comments

@halter73 halter73 halter73 approved these changes

@Tratcher Tratcher Tratcher approved these changes

+1 more reviewer

@pranavkm pranavkm pranavkm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

7.0-preview1

Development

Successfully merging this pull request may close these issues.

Consider a nicer API for configuring JsonOptions for minimal actions / route-to-code

7 participants

@captainsafia @Kahbazi @halter73 @pranavkm @Tratcher @BrennanConroy @amcasey