Skip to content

Conversation

@LuukN2
Copy link

@LuukN2 LuukN2 commented Nov 7, 2018

Various fixes for the type builder.

  • Attributes that take params parameters would cause a cast exception. The expected type was array, but getting the constructor arguments converts params to a list of CustomAttributeTypedArgument.
    Fixed by calling ToArray on the parameter.

  • Some types that were finished still had dependencies on them. Fixed by checking if the type the property is dependent on is present in the dictionary of generated types.

  • Building ODataActionParameters types caused a name conflict when actions with the same namespace and name were defined in other controllers. Fixed by passing the controller name and prefixing the typename with it.

  • Prevent null pointer exception when no ODataActionParameters are found on actions. Having this parameter is not required for actions that take no parameters.

  • Added support for substituting SingleResult, which is an IQuerable with one element.

Still needs test cases but with these fixes I can use the api explorer without any exceptions on the big OData project I'm working with. If everything seems ok, I'll make some test cases.
I can make some repros if needed, let me know.

It still takes around 8 minutes to generate the api descriptions, and I'll be looking in to that after this has been resolved.

@commonsensesoftware
Copy link
Collaborator

This is great. It looks to be in the right direction to me.

@commonsensesoftware
Copy link
Collaborator

Just some quick items:

  1. Is this the final change set or is there more?
  2. Somehow there is branch conflict that I'm not following because the last commits on the remote master have only come from you actually.
  3. For the performance stuff, CLR type lookup can definitely be improved and that can be achieved by checking for and updating the ClrTypeAnnotation on an EDM type as necessary.

@LuukN2
Copy link
Author

LuukN2 commented Nov 8, 2018

Yeah, these should be the final changes for this PR.

I'm not sure either, it said I had conflicts even though my branch was up-to-date.

Right now I have a static dictionary which increases performance drastically, thought it takes up some memory. The profiler also shows that 98% of time spent is in GetClrType.

@commonsensesoftware
Copy link
Collaborator

Sounds good. I'm trying to get Beta 2 out by EOD. Got some other folks waiting on me. I'll see if I can't figure out what the branch issue here is so we can get it in. The performance stuff can come in the RC. Have a look at: https://github.com/OData/WebApi/blob/feature/netcore/src/Microsoft.AspNet.OData.Shared/Formatter/EdmLibHelpers.cs. Specifically, you probably want to use the annotation API to lookup and set the built-in ClrTypeAnnotation rather than maintain a separate dictionary.

@commonsensesoftware
Copy link
Collaborator

Looks like you have merged in your upstream on a working branch instead of rebasing. Not really a big deal. I'll get this merged. Thanks.

@commonsensesoftware commonsensesoftware merged commit ea48ea5 into dotnet:master Nov 9, 2018
@LuukN2 LuukN2 deleted the type-building-fixes branch November 9, 2018 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants