Skip to content
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

Missing type properties in some Entities #1738

Closed
Ndiritu opened this issue Jul 13, 2022 · 6 comments · Fixed by #1739
Closed

Missing type properties in some Entities #1738

Ndiritu opened this issue Jul 13, 2022 · 6 comments · Fixed by #1739
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience

Comments

@Ndiritu
Copy link
Contributor

Ndiritu commented Jul 13, 2022

We currently clean properties with the @odata. prefix by dropping the prefix e.g. @odata.type becomes type.

A derived type's type property will NOT be added to the model since it's parent already contains a property named type (odata type)

Example: microsoftgraph/msgraph-sdk-dotnet@00ffddc#diff-6a2934f9f1e41fcb43a8a28293569fd779095ce3bc9ff09b9f91a1f6ef22d0cdL99-L102
or check SubjectRightsRequest history

Proposed fix:
Doing away with stripping prefixes but instead only replace special characters & camel case?

@Ndiritu Ndiritu added type:bug A broken experience generator Issues or improvements relater to generation capabilities. labels Jul 13, 2022
@Ndiritu Ndiritu self-assigned this Jul 13, 2022
@Ndiritu Ndiritu added this to Kiota Jul 13, 2022
@Ndiritu Ndiritu moved this to Todo in Kiota Jul 13, 2022
@baywet
Copy link
Member

baywet commented Jul 13, 2022

Thanks for reporting this, already on top of it (for Go/Java) could you have a look at PHP with the fix present in #1739 please?

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jul 13, 2022

I think the PR doesn't solve this:

Here's the openAPI spec snippet. There's already a type property here that is a microsoft.graph.subjectRightsRequestType and it's base class Entity has the "@odata.Type" which is cleaned to type. This results in the SubjectRightsRequest model not containing a type property because we find a property with the same name in the parent.

    microsoft.graph.subjectRightsRequest:
      allOf:
        - $ref: '#/components/schemas/microsoft.graph.entity'
        - title: subjectRightsRequest
          type: object
          properties:
            assignedTo:
              anyOf:
                - $ref: '#/components/schemas/microsoft.graph.identity'
              description: Identity that the request is assigned to.
              nullable: true
           ...
            type:
              anyOf:
                - $ref: '#/components/schemas/microsoft.graph.subjectRightsRequestType'
              description: 'The type of the request. Possible values are: export, delete, access, tagForAction, unknownFutureValue.'
              nullable: true
      x-ms-discriminator-value: '#microsoft.graph.subjectRightsRequest'

@baywet
Copy link
Member

baywet commented Jul 13, 2022

Ah thanks for the précision. I didn't get that. So would you argue that we should strip the prefix but only clean the special characters?

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jul 13, 2022

I think we should only clean special characters and not strip the prefix. Stripping the prefix may mean we have to check if the property already exists and de-dupe names.

@Ndiritu
Copy link
Contributor Author

Ndiritu commented Jul 13, 2022

I think such duplicates would only apply in OData to OpenAPI scenarios

Otherwise, I think "traditional" openAPI should take care of duplicate property names from the yml/json validation

@baywet
Copy link
Member

baywet commented Jul 13, 2022

Makes sense. I'll add that to my pull request tomorrow. @darrelmiller any objections? (currently @odata.type gets converted to type for the code symbol and that creates conflicts if the schema already describes a type property)

@baywet baywet moved this from Todo to In Progress in Kiota Jul 14, 2022
Repository owner moved this from In Progress to Done in Kiota Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants