-
Notifications
You must be signed in to change notification settings - Fork 62
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
adds support for properties of complex type and navigation properties on complex types #158
Conversation
…property Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
… paths Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
… request body Signed-off-by: Vincent Biret <[email protected]>
… types Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
.Where(CanFilter)) | ||
{ | ||
var count = _model.GetRecord<CountRestrictionsType>(np, CapabilitiesConstants.CountRestrictions); | ||
RetrieveNavigationPropertyPaths(np, count, currentPath, convertSettings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RetrieveNavigationPropertyPaths
will throw an exception when calling ShouldExpandNavigationProperty
since OdataSegment.EntityType
is not yet implemented:
public virtual IEdmEntityType EntityType => throw new NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for reporting that, can you give it another try with this latest commit please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's related but, a null exception is thrown when attempting to retrieve the "Entity type of a Complex type property segment" here:
if (navEntityType.IsAssignableFrom(segment.EntityType)) |
Example:
The complex property segment in question:
The complex type which has containment navigation properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just pushed another commit that should solve your issue, please pull and give it another try.
…e descriptions Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
5e5c41e
to
104b4cb
Compare
OpenApiSchema schema = new OpenApiSchema | ||
{ | ||
OpenApiSchema schema = new() | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you referring to the new()
syntax?
Basically, it looks good to me. |
test/Microsoft.OpenAPI.OData.Reader.Tests/PathItem/ComplexPropertyPathItemHandlerTests.cs
Show resolved
Hide resolved
@baywet We cannot generate paths for properties that are complex types unless there is a specific annotation to indicate that the path is supported. It causes way too many paths to be generated. |
@darrelmiller this PR resulted in a 3x path items increase. (considering it might be impacted by inaccurate contains target attributes as well) I'd be ok with having a default of "if there's no annotation saying it's supported, don't add anything" and this should be a quick change once we know which annotation to read in. Which annotation should I read to implement that change? |
fixes #15
This pull request adds path items for complex properties on entities as well as path items for navigation properties in complex types. Subsequently, it adds raw count, type cast and $ref path segments under those when required.
For a detail of which paths we're adding, see #15