SAP / odata-vocabularies

SAP Vocabularies for semantic markup of structured data published via OData (www.odata.org) services.
https://sap.github.io/odata-vocabularies
Apache License 2.0
165 stars 60 forks source link

Support annotating common expressions as sort order of entity sets #333

Closed GeraldKrause closed 2 months ago

GeraldKrause commented 2 months ago

This proposal addresses the requirement to use a common expression in the sort order annotated to an entity set. For this purpose, this proposal extends the structure type of term Common.SortOrder.

Rationale for this extension: In the concrete use case of this requirement, EDMX is used to as an interface definition language (a.k.a. service provider interface (SPI)) to describe the sort behavior a service implementation compliant with the API definition must offer for the annotated entity set. Allowing the use of an expression for a sort item mirrors the expressiveness of the protocol ABNF and leads to a self-contained API description that can be confidently published on the SAP Business Accelerator Hub (that is, no need for an out-of-band textual description).

Discussion needed: The type of term Common.SortOrder is reused in UI.PresentationVariant. If there are concerns that this proposal adds to much freedom for services and makes it hard for UIs to interpret Common.SortOrder annotations because they lack a common expression parser, an alternative design would be to keep the term Common.SortOrder as it is today and create a new term Common.SortOrderExpressions with a type derived from Common.SortOrderType adding the expression to this structure.

Example from the use case of the SAP Build Work Zone product: Sort instances of the entities entity set such that instances with identification/entityType = "Site" are returned before all other instances

<Annotations Target="entities">
  <Annotation Term="Common.SortOrder">
    <Collection>
      <Record>
        <PropertyValue Property="Expression">
          <Eq>
            <Path>identification/entityType</Path>
            <String>Site</String>
          </Eq>
        </PropertyValue>
      </Record>
    <Collection>
  </Annotation>
</Annotations>

  …
</Annotations>
ralfhandl commented 2 months ago

Please add an example file examples/Common.SortOrder-sample.xml with your example

BerSie commented 2 months ago

I haven't finished my review, but after reading the first lines of the description I added @nlunets to the review.

nlunets commented 2 months ago

That's sounds like fun.... :)

Currently sort order matches a collection of property + direction in all the use case we have there so having an own property Expression for it make sense.

We'd need to check if backends + the ui5 model actually supports this, however i'm not sure if we need to have the expression created as a complex type instead of a string.

GeraldKrause commented 2 months ago

Thanks @HeikoTheissen dynamic expressions are the so much better approach than a string. Also @BerSie @ralfhandl I updated the example in the description as well.

ralfhandl commented 2 months ago

@nlunets Do you still have concerns?

nlunets commented 2 months ago

I do, or at least i'd be happy to get some counterpoints for the ones I raised above

ralfhandl commented 2 months ago
  • It's harder for people to define

No, it's just a different way to write down expressions.

  • Are we support the entire breadth of the expressions there ? Like if i start to go crazy with if, else, and or concat would that work or not :)

This question is independent of the representation of the expressions, one can go crazy in both equivalent dialects.

First use case is a simple comparison. We will see where we will end up.

Note that Validation.Constraint is already designed this way with a Boolean dynamic expression.

  • The frontend implementations will have again to transform the expressions into the string

True if this is to be sent back to an OData service. Gerald's use case is as an implementation constraint for a service implementation, not a frontend.

GeraldKrause commented 2 months ago
  • It's harder for people to define

No, it's just a different way to write down expressions.

  • Are we support the entire breadth of the expressions there ? Like if i start to go crazy with if, else, and or concat would that work or not :)

This question is independent of the representation of the expressions, one can go crazy in both equivalent dialects.

First use case is a simple comparison. We will see where we will end up.

Note that Validation.Constraint is already designed this way with a Boolean dynamic expression.

  • The frontend implementations will have again to transform the expressions into the string

True if this is to be sent back to an OData service. Gerald's use case is as an implementation constraint for a service implementation, not a frontend.

I fully agree with @ralfhandl. Including again @nlunets - Still, as another option, we could also decide to go with the alternative design I outlined in the PR description. Instead of extending the existing Common.SortOrder term and add this complexity, we could define a new, similar term Common.SortOrderExpression that is not used by the UI and provides the option to use sort expressions.

WDYT?

nlunets commented 2 months ago

Hmmm ok i missed the part where this is more an implementation guideline for the backend than a default $orderby clause to be added in all request...

The fact that the odata specification actually allow for such expression would make this semantically correct to exist on the Common.SortOrder (and maybe one day in the future we could also support it...) so as such the proposal with the SortOrder would also be fine for me.

ralfhandl commented 2 months ago

@BerSie If you also agree, you could merge

BerSie commented 2 months ago

Indeed I'm still a bit hesitant, as the SortOrder is also used in UI.PresentationVariant (as mentioned by @GeraldKrause). So, the advantage of having an own term Common.SortOrderExpression would be, that the presentation variant would not be influenced. But, if FE would support these expressions in future, the PresentationVariant has to be extended. That's why I'm unsure what's the "better" approach". @nlunets: can you estimate how likely a FE implementation is?

nlunets commented 2 months ago

image I'll let you estimate the axis type and unit :)

My main worry is that the backend may not support passing expression as $orderby clause properly, and then if they do, that ui5 doesn't support defining a sorter with a weird expression like this :)

Besides this the parsing / creation of the expression is not that complicated to do I think.

So it's not something that could come tomorrow, but with proper planning, motivation and alignment it should be doable.... one day :)

BerSie commented 2 months ago

... I'll let you estimate the axis type and unit :)

My main worry is that the backend may not support passing expression as $orderby clause properly, and then if they do, that ui5 doesn't support defining a sorter with a weird expression like this :)

Besides this the parsing / creation of the expression is not that complicated to do I think.

So it's not something that could come tomorrow, but with proper planning, motivation and alignment it should be doable.... one day :)

:) ... and one axis should have 0...1 or 0...100% for the likelihood :) So, as you don't exclude it, the extension of the existing term is OK