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

Key fields not required #316

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

HeikoTheissen
Copy link

@HeikoTheissen HeikoTheissen commented Nov 27, 2024

Currently, non-computed key properties are required in OpenAPI. But this ignores the case of key properties that can be either provided or omitted (in which case they are computed by the server).

We have three types of key properties

Annotation of key property Meaning for create request contained in OpenAPI -create schema required
Core.Computed Must not be provided, computed by server no no
Core.ComputedDefaultValue May be provided yes no
Neither Must be provided, validated by server yes yes

This pull request corrects the OpenAPI description for the second type.

Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case with this annotation would be nice. Could be an existing one with the annotation added, I think we have enough "non-computed" key properties in the examples 😎

@HeikoTheissen
Copy link
Author

Common.FieldControl = Mandatory means that the field must have a value for the entity to have a valid state. But this value can be set after creation, or during creation by the server. In any case, such a field is not required in the -create schema.

@ralfhandl
Copy link
Contributor

such a field is not required in the -create schema

This is correct, but someone might miss this only possibility to mark a field as "required" in the create structure.

The correct way to do that would be via Capabilities.InsertRestrictions/RequiredProperties, which is currently not evaluated by this transformation.

What I am trying to express: this correction may disappoint some users, and there is no mitigation.

@HeikoTheissen
Copy link
Author

HeikoTheissen commented Nov 29, 2024

What I am trying to express: this correction may disappoint some users, and there is no mitigation.

That's for the future phase (#291):

<!-- non-computed key properties are required, as are Capabilities.InsertRestrictions/RequiredProperties -->
<xsl:for-each select="$structuredType/edm:Property[
(@Name=../edm:Key/edm:PropertyRef/@Name and not(@id=//edm:Annotation[not(@Qualifier) and
(@p2:Term='Org.OData.Core.V1.Computed' and not(@Bool='false') or
@p2:Term='Org.OData.Core.V1.ComputedDefaultValue' and not(@Bool='false') or
@p2:Term='Org.OData.Core.V1.Permissions' and @p2:EnumMember='Org.OData.Core.V1.Permission/Read' or
@p2:Term='com.sap.vocabularies.Common.v1.FieldControl' and @p2:EnumMember='com.sap.vocabularies.Common.v1.FieldControlType/ReadOnly')]/@target)) or
@Name=//edm:Annotation[not(@Qualifier) and id(@target)/@p1:EntityType=$structuredType/@id and @p2:Term='Org.OData.Capabilities.V1.InsertRestrictions']
/edm:Record/edm:PropertyValue[@Property='RequiredProperties']/edm:Collection/edm:PropertyPath or
@Name=//edm:Annotation[not(@Qualifier) and @p2:Term='Org.OData.Capabilities.V1.NavigationRestrictions']
/edm:Record/edm:PropertyValue[@Property='RestrictedProperties']/edm:Collection
/edm:Record[edm:PropertyValue[@Property='NavigationProperty' and id(@p1:NavigationPropertyPath)/@p1:Type=$structuredType/@id]]/edm:PropertyValue[@Property='InsertRestrictions']
/edm:Record/edm:PropertyValue[@Property='RequiredProperties']/edm:Collection/edm:PropertyPath]">

@HeikoTheissen
Copy link
Author

The correct way to do that would be via Capabilities.InsertRestrictions/RequiredProperties, which is currently not evaluated by this transformation.

If this was strictly evaluated, the outcome would depend on the resource path. But the required belongs to a *-create schema which exists only once per entity type, not per resource path.

@ralfhandl
Copy link
Contributor

the required belongs to a *-create schema which exists only once per entity type, not per resource path

Generating structures per resource path was considered and discarded due to the significantly increased size of the generated OpenAPI files. Given that OpenAPI files now serve as input to LLMs whose context size is still limited, file size will continue to matter for some time.

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