Matching parameter by name when binding ODataQueryOptions. #1173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robward-ms
force-pushed
the
NetCore-1170-1172
branch
from
4177cde
to
ea66dea
Compare
AlanWong-MS
reviewed
Dec 21, 2017
| ? configuration.GetODataRootContainer(routeName) | ||
| : configuration.GetNonODataRootContainer(); | ||
|
|
||
| IServiceProvider rootContainer = configuration.GetODataRootContainer(routeName); |
There was a problem hiding this comment.
nit: the indentation is off by one space from the other lines.
AlanWong-MS
reviewed
Dec 21, 2017
| { | ||
| // For HTTP routes, use the default request services. | ||
| rootContainer = request.HttpContext.RequestServices; | ||
| throw Error.ArgumentNull("routeName"); |
There was a problem hiding this comment.
We're throwing argument null exception for "routeName" when perRouteContainer is null? Should this one be an exception for perRouteContainer and a separate if-statement check above for routeName?
AlanWong-MS
reviewed
Dec 21, 2017
| throw Error.ArgumentNull("routeName"); | ||
| } | ||
| IServiceProvider rootContainer = perRouteContainer.GetODataRootContainer(routeName); | ||
| if (perRouteContainer == null) |
There was a problem hiding this comment.
Duplicate perRouteContainer check here. I think you meant rootContainer?
AlanWong-MS
reviewed
Dec 21, 2017
| perRouteContainer.CreateODataRootContainer(null, ConfigureDefaultServices(builder, configureAction)); | ||
| } | ||
|
|
||
|
|
AlanWong-MS
reviewed
Dec 21, 2017
| IPerRouteContainer perRouteContainer = builder.ServiceProvider.GetRequiredService<IPerRouteContainer>(); | ||
| if (perRouteContainer == null) | ||
| { | ||
| throw Error.ArgumentNull("routeName"); |
There was a problem hiding this comment.
Same question regarding the use of "routeName" as above.
robward-ms
force-pushed
the
NetCore-1170-1172
branch
from
ea66dea
to
e5754c7
Compare
AlanWong-MS
reviewed
Dec 21, 2017
| // determination. | ||
| if (!contentTypeSpecified && matchedMappings != null && matchedMappings.Any()) | ||
| // Now pick the best content type. If a media mapping was found, use that and override the | ||
| // value specified by the controller, if any. Otherwiser, let the base class decide. |
There was a problem hiding this comment.
typo: Otherwiser -> Otherwise
robward-ms
commented
Dec 21, 2017
| /// </summary> | ||
| /// <param name="configuration">The server configuration.</param> | ||
| /// <param name="rootContainer">The root container.</param> | ||
| public static void SetNonODataRootContainer(this HttpConfiguration configuration, |
There was a problem hiding this comment.
This should be internal, not public.
Skip OData output formatting for non-OData routes. Improved routing action selection. Add EnableDependencyInjection() for non-OData routes.
robward-ms
force-pushed
the
NetCore-1170-1172
branch
from
e5754c7
to
d156f5f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues
This pull request makes progress on issues #975, #939, #772, #628, #229.
This pull request fixes #1170, #1171, #1172
Description
Matching parameter by name when binding ODataQueryOptions.
Skip OData output formatting for non-OData routes.
Improved routing action selection.
Add EnableDependencyInjection() for non-OData routes.
Checklist (Uncheck if it is not completed)
Additional work necessary
Refactor Batching for AspNetCore.
Port Unit Tests to NetCore
Port E2E Tests to NetCore & Xunit 2.0