nullability information for properties that are collections of complex or entity types is inconsistent

[baywet]

nullability information for properties that are collections of complex or entity types is inconsistent

[irvinesunday]

In the case of user.appRoleAssignments, the property cannot be nullable = true, and should always be defined because according to the spec.:

A navigation property whose Type attribute specifies a collection MUST NOT specify a value for the Nullable attribute as the collection always exists, it may just be empty.

https://docs.oasis-open.org/odata/odata/v4.0/errata02/os/complete/part3-csdl/odata-v4.0-errata02-os-part3-csdl-complete.html#_Toc406397966

[baywet]

How does this specification apply here? I was commenting the OpenAPI result, and this spec applies to the CSDL input.

[irvinesunday]

How does this specification apply here? I was commenting the OpenAPI result, and this spec applies to the CSDL input.

Aren't we translating the structural properties to OpenAPI based on their attribute values as defined in the CSDL schema? And according to OData rules, a collection nav. prop cannot be nullable, however it can be an empty collection. And also, it cannot have the attribute, Nullable specified. Are you suggesting, then that when translating these properties that are of a collection type to OpenAPI schema properties we write them out as nullable: true? Doesn't this relay incorrect information?

[baywet]

If we take an example : Group.memberOf (collection navigation property)
This won't be returned by default, or if the expand query parameter doesn't include that property.
Which means the payload won't contain it (the property is omitted)
Which means the payload wouldn't validate its schema in the current form.

In a POST scenario, clients might not perform a deep insert, or default the property to an empty array. And the service doesn't need the property to be able to create a group. Which means the payload would again not validate the current schema.

Aren't we translating the structural properties to OpenAPI based on their attribute values as defined in the CSDL schema?

This is a little like asking whether literal or semantic translation is the better one for human languages. Yes this library is in the translation business, but to me having a result that makes sense is more important than being able to map "words" one to one between the source and the result.

[darrelmiller]

We seem to be conflating a few things. The nullable in the example below is unrelated to the nullability of the collection.

identities:
  type: array
  items:
    anyOf:
      - $ref: '#/components/schemas/microsoft.graph.objectIdentity'
      - type: object
         nullable: true

The schema above says the following JSON is valid:

{
  "identities": [
        {... objectIdentity... },
        null,
        {... objectIdentity... }
     ]
}

I'm not sure why we would ever want to allow that, but it isn't related to a collection property being null. The schema above does not say the following is valid.

{
  "identities":  null
}

@baywet

If we take an example : Group.memberOf (collection navigation property)
This won't be returned by default, or if the expand query parameter doesn't include that property.
Which means the payload won't contain it (the property is omitted)
Which means the payload wouldn't validate its schema in the current form.

Having effectively nullable:false will not fail validation if the property is not present. The nullable constraint does not consider properties that are not present. That's what the required constraint does. Therefore, if you want the property to always be present in the payload then you must include it in the required array. We never do this for Microsoft Graph because of OData $select. If you want to allow the property to be present with the value null then you add nullable: true. For OData they are saying that collections can never have the value null.

This presents two interesting questions:

• Should Graph workloads always include collection properties and make them empty?
• Should Kiota clients always create instances of collections that are empty when initializing a model object?

I guess the OData serializer is going to make the first happen automatically. My concern about Kiota is that I don't want the deserialization process to be creating many empty collection objects. It is unnecessary allocations.

[baywet]

Should Graph workloads always include collection properties and make them empty?

Between the OData spec quote from Irvine and your clarification I'd say no:

Selected or default	State	Result Payload
y	null	empty array
n	null	absent
y	collection	array
n	collection	absent

Should Kiota clients always create instances of collections that are empty when initializing a model object?

I'd say no for the reason you've outlined.

Because properties are not required by default, I think that resolves a lot of my initial concern. (sorry about the confusion, thanks for clarifying)

Maybe the only case that might need fixing is the following

identities:
  type: array
  items:
    anyOf:
      - $ref: '#/components/schemas/microsoft.graph.objectIdentity'
      - type: object
         nullable: true

as I'm not sure why would anyone include null in an array?

[irvinesunday]

For the above schema in question to make sense I believe we should use oneOf instead of anyOf in the items:

identities:
  type: array
  items:
    oneOf:
      - $ref: '#/components/schemas/microsoft.graph.objectIdentity'
      - type: object
         nullable: true

Otherwise, how else can we validate that the array can be empty?

[baywet]

This

[]

Would already validate that

identities:
  type: array
  items:
    - $ref: '#/components/schemas/microsoft.graph.objectIdentity'

The only thing the one/any Of with nullable true allows the example above wouldn't allow is this

[null]

But I'm not sure that makes any sense, hence my suggestion to remove the union type all together here. Which I think is also what Darrel was suggestion earlier.
What do you think?

[irvinesunday]

I have had a side chat with @darrelmiller just for more clarity. We are in agreement about removing the anyOf for properties that are collections.