cap-js / cds-typer

CDS type generator for JavaScript
Apache License 2.0
29 stars 10 forks source link

feat/refactor: use only one CSN flavor + other improvements #356

Open stockbal opened 1 month ago

stockbal commented 1 month ago

Hi @daogrady,

proposal to get rid of one of the CSN flavors. I chose to remove the xtended flavor. All in all the required fixes/changes were not many and had some nice side effects.

Tasks

Here is short sample to show the changes to the generated classes (only the important parts are included)

Sample cds schema

namespace bookshop;

entity Books : cuid {
  title      : localized String;
  publishers : Composition of many {
                 key ID      : UUID;
                     name    : String;
                     type    : String enum {
                       self;
                       independent;
                     };
                     offices : Composition of many {
                                 key ID      : UUID;
                                     city    : String;
                                     zipCode : String;
                               }
               }
}

Generated index.ts for namespace bookshop

// enum
const publisher_type = {
  self: "self",
  independent: "independent",
} as const;
type publisher_type = "self" | "independent"

export function _BookAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class Book extends _._cuidAspect(Base) {
    declare title?: string | null;
    declare publishers?: __.Composition.of.many<Books.publishers>;
    // properties to access composition and association to generated table for 'localized' elements
    // #################################
    declare texts?: __.Composition.of.many<Books.texts>;
    declare localized?: __.Association.to<Books.text> | null;
    // #################################
  };
}
export class Book extends _BookAspect(__.Entity) {}
export class Books extends Array<Book> {$count?: number}

export namespace Books {
  // classes for inline composition inside namespace of parent entity
  export function _publisherAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
    return class publisher extends Base {
      declare up_?: __.Key<__.Association.to<Book>>;
      declare up__ID?: __.Key<string>;
      declare ID?: __.Key<string>;
      declare name?: string | null;
      declare type?: publisher_type | null;
      declare offices?: __.Composition.of.many<Books.publishers.offices>;

      static type = publisher_type
    };
  }
  export class publisher extends _publisherAspect(__.Entity) {}
  export class publishers extends Array<publisher> {$count?: number}

  // generated text entity inside namespace of parent entity
  export function _textAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
    return class text extends _sap_common._TextsAspectAspect(Base) {
      declare ID?: __.Key<string>;
      declare title?: string | null;
    };
  }
  export class text extends _textAspect(__.Entity) {}
  export class texts extends Array<text> {$count?: number}

}

export namespace Books.publishers {
  // classes for nested inline composition 'offices'
  export function _officeAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
    return class office extends Base {
      declare up_?: __.Key<__.Association.to<Books.publisher>>;
      declare up__ID?: __.Key<string>;
      declare up__up__ID?: __.Key<string>;
      declare ID?: __.Key<string>;
      declare city?: string | null;
      declare zipCode?: string | null;
    };
  }
  export class office extends _officeAspect(__.Entity) {}
  export class offices extends Array<office> {$count?: number}
}

Generated index.js for namespace bookshop

// Books
module.exports.Book = { is_singular: true, __proto__: csn.Books }
module.exports.Books = csn.Books
// Books.publishers
module.exports.Books.publisher = { is_singular: true, __proto__: csn.Books.publishers }
module.exports.Books.publishers = csn.Books.publishers
// Books.texts
module.exports.Books.text = { is_singular: true, __proto__: csn.Books.texts }
module.exports.Books.texts = csn.Books.texts
// Books.publishers.offices
module.exports.Books.publishers.office = { is_singular: true, __proto__: csn.Books.publishers.offices }
module.exports.Books.publishers.offices = csn.Books.publishers.offices
// events
// actions
// enums
module.exports.publisher.type ??= { self: "self", independent: "independent" }

This branch is again based on the fix for the draftable state (see #348)

Fixes #116 Closes #77 Closes #128

Let me know if you want to go forward with this branch/approach.

Regards, Ludwig

daogrady commented 1 month ago

Hi Ludwig,

very impressive work, thanks for putting in the effort! As this is quite the intrusive change, I'd like to give this a more thorough look. I will be out of office for two weeks, starting Friday, so I won't be able to look into this (or other PRs) until mid November. Just so you know this is not being shelved, just put on hold until I have the time to give it a proper review. 🙂

Best, Daniel

daogrady commented 1 week ago

One particular problem we formerly overcame with our two-flavour-approach was dealing with the provenance of annotations. This is, as far as I am aware, generally an unsolved problem when you don't have access to the xtended flavour. Consider the following model:

@singular: 'A'
entity A {}

entity B: A {}

Using only the inferred flavour, both A and B would have the @singular: 'A' annotation. The generated code would therefore contain a duplicate class A:

// This is an automatically generated file. Please do not change its contents manually!
import * as __ from './_';

export function _AAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class A extends Base {
    static readonly kind: "entity" | "type" | "aspect" = 'entity';
    declare static readonly keys: __.KeysOf<A>;
    declare static readonly elements: __.ElementsOf<A>;
    static readonly actions: Record<never, never>;
  };
}
export class A extends _AAspect(__.Entity) {}
Object.defineProperty(A, 'name', { value: 'A' })
Object.defineProperty(A, 'is_singular', { value: true })
export class A_ extends Array<A> {$count?: number}
Object.defineProperty(A_, 'name', { value: 'A' })

export function _AAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class A extends _AAspect(Base) {
    declare x?: number | null
    static override readonly kind: "entity" | "type" | "aspect" = 'entity';
    declare static readonly keys: __.KeysOf<A>;
    declare static readonly elements: __.ElementsOf<A>;
    static readonly actions: typeof A.actions & Record<never, never>;
  };
}
// v ❌ v
export class A extends _AAspect(__.Entity) {}
Object.defineProperty(A, 'name', { value: 'B' })
Object.defineProperty(A, 'is_singular', { value: true })
export class B extends Array<A> {$count?: number}
Object.defineProperty(B, 'name', { value: 'B' })

did you consider this aspect already or do you see a way to handle this with only the inferred flavour available?

stockbal commented 1 week ago

To be honest I didn't think of that. Considerung your sample I personally would say that such entity inheritence scenarios are not very realistic. Until now, I only encountered the use of aspects to enrich other entities with a set of annotations or fields (e.g. cuid, managed, CodeList to name a few). In that case we would be safe as one would not add @singular or @plural to an aspect.

That being said, the inferred model contains the annotations according to the rules of annotation propagation. In the past I personally had some issues because we had to annotate our entities in the database model and again in the service because of the missing propagation of @singular/@plural.

So there a times a propagation of @singular/@plural is welcome and times where it is not.

e.g.

// schema.cds
namespace db;

// -> MyBooks, MyBook
@singular: 'MyBook'
@plural: 'MyBooks'
entity Books {}

// -> Pubs, Pub
@singular: 'Pub'
@plural: 'Pubs'
entity Publishers {}

// -> BestsellingBooks, BestsellingBook
@singular: null // stop propagation and keep original name of entity
@plural: null // stop propagation and keep original name of entity
entity BestsellingBooks as select * from Books {}
// service.cds
using {db} from '../db/schema';

service MyService {
   // -> Books, Book
   @singular: null // stop propagation and keep original name of entity
   @plural: null // stop propagation and keep original name of entity
   entity Books as projection on db.Books;

   // -> BestsellingBooks, BestsellingBook (annotation propagation was stopped)
   entity BestsellingBooks as projection on db.BestsellingBooks;

   // -> Pubs, Pubs (via propagation)
   entity Publishers as projection on db.Publishers;
}

At the end, it is of course a breaking change and users would need to adjust their models to correct their generated types, but cds-typer would follow the documented rules of how annotations are propagated. At times it may force users to add more annotations to their models as before.

So 1) keep the xtended model for knowing the true origin of @singular/@plural and spare the users some thinking/writing or 2) add some additional documentation so it is clear how the annotations would affect the generated types. I don't think there is a good third option.

daogrady commented 4 days ago

In that case we would be safe as one would not add @singular or @plural to an aspect.

🤔 why is that? Aspects will be generated as classes by cds-typer, just as entities are. They therefore suffer from the same shortcomings our current inflection algorithm has, and thus can be annotated with custom inflection.

In the past I personally had some issues because we had to annotate our entities in the database model and again in the service because of the missing propagation of @singular/@plural.

not sure I grasp that either. Are you talking about having to repeat the naming for service-entities, which bears the risk of being inconsistent during renamings?

As for your proposed solutions: I agree, as I don't see any third solution either. I will bring this up in the next cds-typer sync and see if there is any preferred way forward.

stockbal commented 2 days ago

🤔 why is that? Aspects will be generated as classes by cds-typer, just as entities are. They therefore suffer from the same shortcomings our current inflection algorithm has, and thus can be annotated with custom inflection.

The array class type is not generated for those, only for entities. Therefore I am not even sure why inflection is required for aspects at all. For example an aspect Status will result in a class Statu right now. Which would make sense for an entity type because otherwise the array class would have the same name.

e.g.:

// model.cds
aspect Status {}

entity MyStatus : Status { }
// index.ts
export class Statu extends _StatuAspect(__.Entity) {}

export class MyStatu extends _MyStatuAspect(__.Entity) {}
export class MyStatus extends Array<MyStatu> {$count?: number}

not sure I grasp that either. Are you talking about having to repeat the naming for service-entities, which bears the risk of being inconsistent during renamings?

Exactly. I added the annotations on db level because of unsatisfactory inflected names and then had to repeat the annotations again on service level. Consistent renaming in both db and service model was actually not an issue, at least not yet. We just expected the annotations to be inherited to services level, because of the mentioned propagation