fallback to a sane default when edm parameters and clr parameters disagree #478
Conversation
|
|
|
@commonsensesoftware Once #502 is resolved can you take a look at this? |
|
Sure thing. I've been really backed up lately. |
|
Apologies for the long delay. I'm finally circling back around to this. I'm not sure I fully understand the scenario that this is addressing. What exactly do you mean by:
Are you referring to action parameters that are not part of the EDM? Those should be added and processed as query string parameters, unless I'm missing something. I'm trying to get a better understanding of the situation to make sure we're going down the right path. It appears that the parameter names are not matching up. All action parameters should be a superset of any parameters defined by the EDM action or function. OData has some advanced edge cases so I won't be all that surprised if I missed something. Thanks. |
|
So one of the tasks that my calling convention does, is translation of some odata query operators into parameters, This ends up meaning that the parameters declared to the EDM are not the same as the ones declared to the CLR. Specifically I end up passing the parameters into a single attributed argument that contains all of the things declared to the EDM. So its not a case of having fewer EDM arguments, its that I have a bunch of EDM arguments and no matching CLR arguments. |
|
Sounds kind of like custom model binding. Since there aren't any new tests that illustrate your scenario, can you provide the world's simplest example here? I'm trying to wrap my small squirrel brain around exactly what this looks like. Support for documenting query options is relatively new. I guess I never really considered a scenario as wild as translating the parameters to mean something else, but I suppose that's valid. There may need to be a configuration option to not document query options in this type of setup. |
|
I have some docs on this mechanism here https://github.com/Synergex/HarmonyCore/wiki/Adapters does this provide enough of an example? |
|
That definitely provides some context - interesting. It would appear that this framework calls Let me digest all of this and see if I don't have any more questions. |
| @@ -222,12 +222,14 @@ void AppendParametersFromConvention( StringBuilder builder, IEdmOperation operat | |||
| var actionParameters = Context.ParameterDescriptions.ToDictionary( p => p.Name, StringComparer.OrdinalIgnoreCase ); | |||
| var parameter = parameters.Current; | |||
| var name = parameter.Name; | |||
| actionParameters.TryGetValue( name, out var mappedParameter ); | |||
There was a problem hiding this comment.
I'm still not sure I 100% grock the full use case, but I don't see anything glaring where this change is specific to your scenario. It appears to be a rational fallback, even if it shouldn't happen.
Let's simply change the entire setup to:
var routeParameterName = name;
if ( actionParameters.TryGetValue( name, out var mappedParameter ) )
{
#if WEBAPI
routeParameterName = mappedParameter.ParameterDescriptor.ParameterName;
#elif API_EXPLORER
routeParameterName = mappedParameter.ParameterDescriptor.Name;
#else
routeParameterName = mappedParameter.Name;
#endif
}| @@ -239,12 +241,14 @@ void AppendParametersFromConvention( StringBuilder builder, IEdmOperation operat | |||
| { | |||
| parameter = parameters.Current; | |||
| name = parameter.Name; | |||
| actionParameters.TryGetValue( name, out mappedParameter ); | |||
There was a problem hiding this comment.
The change down here should obviously be the same.
|
Seems like you might be busy. I've incorporated the same changes in #521 so I'm going to close this PR out. I'm about to start the release build so I can publish the packages that will include this change. Thanks. |
|
thanks, sorry for the slowness. |
|
No problem. It was small change so I just lifted it over for you. The latest packages with this change have been published. Cheers! |
I have a custom odata routing convention to deal with optional parameters, unfortunately this leaves me in a position where the clr parameters disagree with the declared edm parameters. The routing convention will dispatch and translate where appropriate but attempting to build the swagger doc fails as a result. I have an additional PR that i will be sending to the odata\odata.net repo, The odata path templates produced in this workflow end up having a slightly different shape that just needed to be handled.