[BUG] Missing "drafts" property to register handlers for draft enabled service

[stockbal]

Hi,

Currently it is not possible to use the generated types to register handlers specific to draft entities.
Let's say we have following service file:

using my.bookshop as my from '../db/data-model';

service CatalogService {
    @odata.draft.enabled
    entity Books as projection on my.Books;
}

In the service handler file I would just like to use the following approach to register my handler for the draft entity Books.

import { ApplicationService } from "@sap/cds";
import { Books } from "#cds-models/CatalogService";

export class CatalogService extends ApplicationService {
  override async init(): Promise<void> {
    this.after("READ", Books.drafts, (books) => {
      // ...
    });
    await super.init();
  }
}

Possible solution

Enhance the generated types for draft enabled service entities in the following way:

import { Definition } from '@sap/cds/apis/reflect';
...
export class Book extends _._managedAspect(_._cuidAspect(_BookAspect(__.Entity))) {
  static drafts: Definition
}
export class Books extends Array<Book> {
  static drafts: Definition;
}

This would solve the missing drafts property and would allow the handler registration. You would not get type inference in the data property unfortunately and a cast would be necessary.

this.after("READ", Books.drafts, (books: Books) => { });

I am not sure if both features can be realized at once. But I personally could live with the cast 😊.

Regards,
Ludwig

Details

Versions of CDS tools[^3]:

cap-type-generator
Node.js	v18.17.0
@sap/cds	7.0.3
@sap/cds-compiler	4.0.2
@sap/cds-dk	-- missing
@sap/cds-dk (global)	7.0.3
@sap/eslint-plugin-cds	2.6.3
@sap/cds-mtxs	1.9.2

Version of cds-typer[^1]: 0.5.0

[daogrady]

Hi Ludwig,

thanks for reporting this issue! I will confer with the runtime team and look into this asap.

Best,
Daniel

[daogrady]

Just a quick idea, I believe

export class Book extends _._managedAspect(_._cuidAspect(_BookAspect(__.Entity))) {
  static drafts: typeof Book
}

should be what you are looking for and should retain proper type support, is that correct?

[stockbal]

Yes, that's it.
Then adding the Definition type is not necessary at all.
But it would be good to add the static property to both types, so Book and Books.

[daogrady]

Hi Ludwig,

just wanted to give you a quick update on this: I have implemented the change in a PoC, but I still need to confer with the runtime team on this, of which many are currently in their well-deserved holiday break. So I have not forgotten this, it just needs to go through some planning, approval, etc. And I didn't want this issue to just become stale. 🙂

Best,
Daniel

[stockbal]

Hi Daniel,

thanks for the update.
I saw in you PoC that you checked for the annotation @fiori.draft.enabled. I think this one is just to enable draft support for localized data (see https://cap.cloud.sap/docs/advanced/fiori#draft-for-localized-data). The correct one to check should be @odata.draft.enabled.

The other thing I noticed is, that you only add the drafts property to the entity that is directly annotated. This does not take into account any draft enabled child entities.
e.g.:

// model.cds
entity Books : cuid {
  title    : String;
  channels : Composition of many DistributionChannels on channels.book = $self;

entity DistributionChannels : cuid {
  channelName : String;
  book        : Association to Books;
}

// service.cds
service CatalogService {
  @odata.draft.enabled
  entity Books as projection on my.Books;

  // will be draft enabled as well
  entity DistributionChannels as projection on my.DistributionChannels;
}

I experimented a bit with the cds compile -2 csn command but I could not find any indication on the child entity that it is draft enabled as well. The only solution I can think of is to look if an entity is used as a composition in an entity that is annotated with @odata.draft.enabled, or you add the annotation manually to the child entity's csn model when the parent entity is processed.

What do you think? 😊

Regards,
Ludwig

[daogrady]

Hi Ludwig,

thanks for poking around and the feedback on your findings!

The correct one to check should be @odata.draft.enabled.

You are right, this would be the more correct way to do it. I think there is even some _isDraftEnabled property somewhere hidden away, which would probably hold the "actual" truth.

I could not find any indication on the child entity that it is draft enabled as well.

There is a way to propagate annotations down to child entities by using the inferred flavour of CSN, but as cds-typer uses the xtended flavour, this is sadly not an option. We do already have a (actually probably obsolete) mechanism in place to touch up on annotations. So we could propagate this information down ourselves.
But we would like to stick as closely as possible to the generated CSN without adding too much "smart logic" into the type generation process. So we will ask the runtime team if it is possible to make information like being draft-enabled a bit more immediate. So we probably have to park it one way or another.

Best,
Daniel

[daogrady]

Hi Ludwig,

I have conferred with the project's PO on this and we have decided to implement this feature despite requiring some special handling.
The draftability of an entity is now dependent on the @odata.draft.enabled annotation, which is propagated down in the inheritance.
If you'd like, you can check the PR branch to see if it handles your use case well.

Best,
Daniel

[stockbal]

Hi Daniel,

I checked the PR and I have few remarks 😊.

Singular drafts property on array types

I noticed that the drafts property on the generated array types is using the singular type
e.g.

export class Books extends Array<Book> {static drafts: typeof Book}

If would change this to drafts: typeof Books for the following reasons:

1. It would better match the plurality of the key word drafts
2. Service handlers like after are always returning an array now, so if the drafts property is typed to the Book class this would be cause a wrong type inference.
   

Draft inheritence on compositions

As already mentioned here #34 (comment), compositions of draft enabled entities are automatically draft enabled as well. This is not yet reflected in the PR.

Regards,
Ludwig

[daogrady]

Hi Ludwig,

thank you for your insights! I am not very draft-savvy myself, so your feedback it highly valued to work out the details. 🙂

Draft inheritence on compositions

I am not quite clear on the composition-semantics you are describing:

compositions of draft enabled entities are automatically draft enabled

look if an entity is used as a composition in an entity that is annotated with @odata.draft.enabled

So in which of the following two examples would you expect the annotation to be propagated to the other entity?

@odata.draft.enabled
entity A {}

entity B {
  a: Composition of A
}

entity A {}

@odata.draft.enabled
entity B {
  a: Composition of A
}

Singular drafts property on array types

This is actually something that still needs some rectification on the side of the cds API. The types for the, say, CRUD handlers currently derive the nature of the req parameter in the handler automatically:

.after('READ', Book, ({ data }) => ... )
//              \      /
//         implicitly the same
.after('READ', Books, ({ data }) => ... )
//              \       /
//         and here as well

but that does not properly reflect the current runtime behaviour. For example in the case you pointed out in .after, data will be an array of entity instance and therefore Books in most cases¹. The types will have to reflect that, no matter whether we call .after('READ', Book, ...) or .after('READ', Books, ...).

Long story short: the type used for the draft property is not (supposed to be) determining to the behaviour of the handlers. That is something the API has to work out. I think it therefore make sense to uniformly pass in the singular type² to handle fewer cases in the types for .after.

Best,
Daniel

Footnotes

1. this actually solely depends on the type of service we are writing the handlers for. ↩

2. or uniformly use the plural type if we want to please linguistic aficionados, but not both. ↩

[lewingan]

Hi Daniel
I am also keen to see this drafts feature in typer as our development uses drafts quite extensively in CAP.

If I may chime in on the question regarding which entities should be draft enabled. I believe it is your second example where as a result of the parent entity B being annotated as draft, that child entity A will be draft enabled as well.

entity A {}

@odata.draft.enabled
entity B {
  a: Composition of A
}

And I don’t think this would change how you propagate the annotations but typically we would annotate drafts on a projection of an entity, rather then in the main entity definition (per examples also given by Ludwig a few comments up).

Cheers
Lewin

[daogrady]

Hi Lewin,

thanks for weighing in on this! This implies that it's actually kind of complicated to determine if an entity is draft enabled or not. I will try to get a hold of one of the runtime colleagues and see if I can somehow access the elusive _isDraftEnabled method, which should be doing exactly this check. I don't think it makes a lot of sense for me to re-implement this check that already exists elsewhere.
But seeing that there is obviously quite some demand for draft support, I will make sure to prioritise this feature, so that the two of you can hopefully get on with your projects asap. 🙂

Best,
Daniel

[daogrady]

Hi everyone,

being referenced by a draft-enabled entity now causes the referenced entity to become draft-enabled as well. I am still poking around if I have gotten the semantics right, but you are welcome to apply the latest version of the respective feature branch to your model and let me know if something is missing.

Best,
Daniel

[stockbal]

Hi Daniel,

I checked the feature branch and saw that you consider both associations and compositions as draft enabled if the parent is draft enabled.
In reality only a composition relation will convert the child entity into a draft enabled entity.

Regards,
Ludwig

[daogrady]

Hi Ludwig,

thanks again for the feedback! I guess it shows that I have not touched drafts a lot myself. 🙂
I also found that projections may interfer with draftability as well. Will take care of both projections and associations.

Best,
Daniel

[daogrady]

Hi everyone,

draft-enablement is more of a rabbit hole than I had anticipated.
Drafts are now aggressively propagated via aspects, projections, as well as compositions.
The unit tests yield the results I expect and my larger test models also look good.
I have explicitly excluded draft enablement via @fiori.draft.enabled from the scope of the PR addressing this issue, as that has additional ramifications that have to be evaluated separately.

I believe I have emulated¹ the compiler's behaviour properly. If you find that your model still doesn't contain the appropriate properties, please let me know and maybe even include your model so I can include that in my local test pipeline.

Best,
Daniel

Footnotes

1. explicitly involving another compile step is a solution I am trying to avoid, as it would add significant overhead to the typing process, tripling runtime for single typer runs. ↩

[stockbal]

Hi Daniel,

I tested it again, but unfortunately I did not get the required types.
I think there still is a conceptional error in your logic.

Let's take the following db schema and service definition.

/db/data-model.cds

entity Books : managed, cuid {
  title      : String;
  publishers : Composition of many Publishers
                 on publishers.book = $self;
}

entity Publishers : managed, cuid {
  name : String;
  book : Association to Books;
}

/srv/catalog-service.cds

using my.bookshop as my from '../db/data-model';

service CatalogService {
    @odata.draft.enabled
    entity Books      as projection on my.Books;
    // also valid and produces also a draft enabled service entity
    // entity Books as select * from my.Books;

    entity Publishers as projection on my.Publishers;
    // also valid and produces also a draft enabled service entity via composition
    // entity Publishers as select * from my.Publishers;
}

This is a common model/service that can be used with a Fiori Element LR/OP floorplan. With the correct annotations we would be able to edit a Book entity together with multiple Publishers. All draft enabled.

At the current PR state I would get the following draft-enabled types
From the database model:

• Book
• Books
• Publisher
• Publishers

From the service:

• Book
• Books

The drafts properties in the db generated types are not usable at all, because they do not exist during runtime and actually have the value undefined (e.g. when used in a SELECT.from(Books.drafts query), which kind of makes sense as the annotation is in namespace odata, so why would drafts exist on a database entity.

The drafts properties in the service types are correct but not complete, as the types Publisher and Publishers do not have them.

In summary I would say that the drafts property should only be created in the types of service entities.

When should a type of a service entity get the drafts property?

• When service entity is annotated with @odata.draft.enabled
• When service entity is projection of or selecting from a draft enabled entity (see catalog-service.cds sample at the beginning)
• When service entity is used in a composition of a draft enabled entity (no matter where the composition is modelled)

Hope that helps.

Regards,
Ludwig

[daogrady]

Hi Ludwig,

thank you for the extensive writeup! I missed the case of draft-enablement travelling up through projections as in your projection Publishers in your service. I had only considered the enablement being pushed down, as in your projection on Books. 🙂
That should actually be an easy addition which I will add first thing in the morning tomorrow.

Cheers,
Daniel

[daogrady]

Hi Ludwig,

I have added the upwards propagation and the model you have provided looks fine with that change. I am currently working through some more models to see if they look as expected. If you have additional models with an added expectation, feel free to add them here, so I can verify that they're also receiving the appropriate draftability.

Best,
Daniel

[stockbal]

Hi Daniel,

sorry, I did not have time to look into the changes until now.
The drafts property in the generated types on service level look fine now.

You still have the drafts property on database schema level. As I wrote in my previous comment, I am not sure if they should be generated. It could lead to errors as these drafts properties will always be undefined during runtime.

One could argue the types from CDS are "unclean" as well, in that regard that drafts is also available if an entity is extracted from the db service.

e.g.

const db = await cds.connect.to("db");
// 'Books' from /db/data-model.cds in namespace 'my.bookshop'
const dbBooks = db.entities["Books"]; 
//             \
//              type is 'entity' from @sap/cds/apis/csn.d.ts

// will always print 'undefined' for db entities
console.log(dbBooks.drafts);

I personally would welcome it if the drafts property is only available in these types when it is also available during runtime.

Regards,
Ludwig

[daogrady]

Hi Ludwig,

don't worry, I was still testing around myself, so no pressure anyway! 🙂

I forgot to address your concerns about the drafts property being present on DB-entity level, sorry. I actually brought that up in a sync with the runtime team and we agreed that at least for a first version, having an additional property the users would hopefully not implement against is preferable to lacking the property in relevant entities altogether.
As you have observed, even the drafts property itself is not a surefire way to tell if something is draft-enabled, so the approach already has clay feet.

Still, I absolutely see your point that it might be misleading in some cases. So maybe we can approach this in two steps:
First, I'd like to make sure the .drafts property is available where users expect to find it (i.e. #41), so users are no longer blocked by the lack thereof.
I would then add a BLI for improving to which entities .drafts is attached and how it is detected. I will also bring this up with the compiler team so we may hopefully see a clear indicator for draftability without doing the CSN magic I am performing right now.¹
Would this be agreeable with you?

Best,
Daniel

Footnotes

1. fair warning: this could take a while. There are improvements for this on the roadmap as far as I know, but I believe it is not top-priority. ↩

[stockbal]

Hi Daniel,

sounds good to me 😊👌.

Regards,
Ludwig

[daogrady]

Hi Ludwig,

I have just released version 0.9.0 to NPM which contains #41.
The improvements we talked about will be tracked in #68

Thank you again for your great input and repeated detailed writeups. They were really helpful in smoothing out the edge cases and highly appreciated. 🙂

Best,
Daniel