feat/refactor: dedicated classes for texts and inline comps #356

stockbal · 2024-10-12T13:15:35Z

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.

working dedicated classes for inline compositions
working dedicated classes for localized elements

Tasks

add test cases

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`

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 {
  // enum
  const publisher_type = {
    self: "self",
    independent: "independent",
  } as const;
  type publisher_type = "self" | "independent"

  // 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.Books.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 · 2024-10-23T13:19:14Z

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

lib/visitor.js

daogrady · 2024-11-14T14:02:10Z

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 · 2024-11-14T20:40:13Z

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 · 2024-11-19T08:42:02Z

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 · 2024-11-20T22:36:28Z

🤔 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

daogrady · 2024-11-26T08:38:13Z

Hi Ludwig,

we discussed your proposal internally and we would very much like to have this change merged. Especially the simplified CSN handling, inline compositions, and localization are a great benefit in themselves.
I think we should for now keep the xtended flavour for looking up the location of the inflection annotations. At the same time, we acknowledge the inconvenience of "double book keeping" these annotations cause. We hope to progressively phase out the xtended flavour in future development.

Is that an acceptable way forward for you?

Best,
Daniel

stockbal · 2024-11-26T19:53:26Z

Hi Daniel,

that's really great news 😊.
One idea about keeping the xtended csn for the inflections. I could do a very early propagation of the singular/plural annotations, i.e. remove them from the new primary model - the inferred one - and transfer them from the xtended model. After that, the xtended model instance could be discarded. I could do this inside the constructor of the Visitor class.
What do you think about that?

Regards,
Ludwig

daogrady · 2024-11-27T07:05:28Z

Hi Ludwig,

that sounds like another step to driving back xtendend to just the initialisation and streamlines the use of one CSN flavour during the remaining generation process. So sounds good to me! 🙂

Best,
Daniel

daogrady · 2024-12-04T12:42:02Z

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

I just realised there seems to the hypothetical szenario of adding @singular/ @plural to aspects after all. Assuming the generated singular or plural form of an aspect collides with the name of an existing entity, the user would have to add these annotations.
I believe the approach you are currently following is still robust against generating the inflected forms to cds aspects, correct?

(my heartfelt condolences for the GitHub user plural who receives a notification whenever I forget to put @plural in backticks)

daogrady · 2024-12-12T06:51:28Z

Hi Ludwig,

quick update: we discussed this internally and agree with your proposed solution.

Best,
Daniel

stockbal · 2024-12-12T12:01:28Z

Hi Daniel,

ok...
And with my proposed solution you mean synching the namespace, or leaving it as is? 😊.

Regards,
Ludwig

daogrady · 2024-12-12T12:30:31Z

Hi Ludwig,

sorry, I meant the post I had already agreed to before, but now got confirmation of the PO as well:

I absolutely see your concerns and yes, in the worst case, users would end up with up to three names for one entity in their model (singular, plural, and the namespace to top it off). But to me, the path forward you proposed makes sense. Still, I should be able to get a hold of some higher-ups today, so I can also try to get their input on the matter.

Best,
Daniel

stockbal · 2024-12-12T14:24:27Z

Hi Daniel,

sorry, I am still at a loss as to if I still have an open task in this PR or not😅.

P.S.: Maybe I am little slow today as I was sick the last few days.

Regards,
Ludwig

daogrady · 2024-12-17T06:46:44Z

Hi Ludwig,

sorry for the confusion again. 😃
Please leave the namespace for scoped entities at the original name as it is found in the CSN and do not apply any @singular/ @plural rename to it.

Best,
Daniel

stockbal · 2024-12-17T22:41:22Z

Then this PR is ready for review now

daogrady · 2024-12-19T12:33:29Z

Hi Ludwig,

thanks for finishing this feature! I'm afraid I won't be able to give this a proper review before the holidays, as other tasks unfortunately currently take precedence, but if I don't get to it today or tomorrow, this will be one of the first things on my plate in the new year.

Best,
Daniel

daogrady

Thank you for this contribution!
Looks very good and even if it didn't bring support for locale classes, having weeded out several huge "FIXME this is a hack!" blocks makes this a valuable change.

I have added a few remarks and mostly annotated them with severity markers. Especially the syntax ones are largely dust to me.

lib/csn.js

lib/file.js

lib/printers/javascript.js

daogrady · 2025-01-08T07:04:26Z

lib/resolution/entity.js

+    #inheritedElements = null
+
+    /** @returns set of inherited elements (e.g. ID of aspect cuid) */
+    get inheritedElements() {


Have you considered property renaming in this context? I.e., does your code exhibit well-defined behaviour for when a property is present in a parent, but also in the entity itself (but possibly with a changed type)?

entity A { name:String } entity B: A { name:Integer }

As of today, this is a known restriction. Even if you do not explicitly solve the issue, this could be an opportunity to error out if we find redefined properties.

No I haven't considered them - why would anyone do that 😶‍🌫️. No proper solution comes to mind right away, I would have to think about it some more.
If I understand you correctly I could implement the following approach though, to error out and stop the type generation in case of a type mismatch.

// visitor.js - #aspectify for (let [ename, element] of Object.entries(entity.elements ?? [])) { const inheritedElem = inheritedElements?.get(ename) if (inheritedElem) { const type = stringifyType(element.type) const inheritedElemType = stringifyType(inheritedElem.type) if (stringifiedType === inheritedElemType) { continue } else { throw new Error(`Type '${type}' of element '${entity.name}.${ename}' does not match the type '${inheritedElemType}' from the parent`) } } }

I don't know what the architectural reasons would be, but it has happened in the past. 🙂

P.S.: I asked this mainly out of curiosity. As it is beyond the scope of your PR, this can totally be done at a later point in time and by someone else.

Then I would actually table this for later if this is ok for you

Absolutely! 🙂

lib/resolution/resolver.js

lib/resolution/entity.js

lib/visitor.js

daogrady

Thank you for this contribution!
Looks very good and even if it didn't bring support for locale classes, having weeded out several huge "FIXME this is a hack!" blocks makes this a valuable change.

I have added a few remarks and mostly annotated them with severity markers. Especially the syntax ones are largely dust to me, so feel free to just mark them as resolved without change if you feel strongly about your syntax choices.

daogrady · 2025-01-09T13:53:55Z

@stockbal seeing that all immediate points have been addressed, do you consider this PR done and ready for merge, or do you need to make any more changes?

stockbal · 2025-01-09T14:18:34Z

No, unless you still found something that I should change, the MR is done from my point of view.

daogrady · 2025-01-09T14:20:26Z

Great! Then thanks once more and let's merge 🚀

stockbal added 11 commits October 10, 2024 21:10

refactor: overhaul/fix creation of drafts properties in entities

07dbaf8

refactor: rename cds file that causes transpilation errors

8260c32

fix: fixes lint issues

8d2b697

refactor: remove xtended csn

6050b06

fix: transfer key value of associations to foreign key element

ac1552f

fix: do not skip inherited elements for views/projections

1c5844c

fix: fixes inline composition resolution

383b541

fix: fixes scoped entities

cf955ff

feat: include texts entities

9e48f81

test: adjust tests to handle entities in subnamespacees

61a429f

refactor: remove unused imports

935995e

stockbal changed the title ~~feat/refactor: use one csn only + other improvements~~ feat/refactor: use only one CSN flavor + other improvements Oct 12, 2024

Merge branch 'main' into refactor/one-csn

4b8f1ec

daogrady reviewed Nov 14, 2024

View reviewed changes

lib/visitor.js Outdated Show resolved Hide resolved

stockbal added 2 commits November 23, 2024 18:40

Merge branch 'main' into refactor/one-csn

a2034a2

fix: imports/exports for esm mode

3c8c5b4

stockbal added 5 commits December 1, 2024 18:04

Merge branch 'main' into refactor/one-csn

f55e26f

feat: propagate inflection annotations from xtended

dc56572

fix: fix output test

2d00a01

docs: add preliminary changelog

f2d462a

feat: print inline enum to subnamespace buffer if necessary

6d17ed5

stockbal added 3 commits December 6, 2024 21:13

refactor: cache inherited elements of entity inside repository

f4080c3

test: add/enhance test for inline compositions

e1957ed

Merge branch 'main' into refactor/one-csn

a71c475

stockbal marked this pull request as ready for review December 6, 2024 20:32

Merge branch 'main' into refactor/one-csn

d17f8ba

stockbal added 2 commits December 17, 2024 23:24

Merge branch 'main' into refactor/one-csn

4b0f3d3

chore: update changelog

7e8d370

daogrady added the keepalive Will not be closed by Stale bot label Dec 19, 2024

daogrady mentioned this pull request Jan 7, 2025

Properly handle reference types to string fields for enums #452

Open

Merge branch 'main' into refactor/one-csn

9b47593

daogrady changed the title ~~feat/refactor: use only one CSN flavor + other improvements~~ feat/refactor: dedicated classes for texts and inline compositions + refactoring Jan 8, 2025

daogrady changed the title ~~feat/refactor: dedicated classes for texts and inline compositions + refactoring~~ feat/refactor: dedicated classes for texts and inline comps Jan 8, 2025

daogrady requested changes Jan 8, 2025

View reviewed changes

stockbal added 2 commits January 8, 2025 10:33

refactor: resolve review remarks

8ac088c

refactor: resolve review remarks

c6ea598

Merge branch 'main' into refactor/one-csn

d1aa000

daogrady self-requested a review January 9, 2025 14:20

daogrady approved these changes Jan 9, 2025

View reviewed changes

daogrady merged commit d32426a into cap-js:main Jan 9, 2025
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/refactor: dedicated classes for texts and inline comps #356

feat/refactor: dedicated classes for texts and inline comps #356

stockbal commented Oct 12, 2024 •

edited

Loading

daogrady commented Oct 23, 2024

daogrady commented Nov 14, 2024

stockbal commented Nov 14, 2024 •

edited

Loading

daogrady commented Nov 19, 2024

stockbal commented Nov 20, 2024

daogrady commented Nov 26, 2024

stockbal commented Nov 26, 2024

daogrady commented Nov 27, 2024

daogrady commented Dec 4, 2024 •

edited

Loading

daogrady commented Dec 12, 2024

stockbal commented Dec 12, 2024 •

edited

Loading

daogrady commented Dec 12, 2024

stockbal commented Dec 12, 2024 •

edited

Loading

daogrady commented Dec 17, 2024

stockbal commented Dec 17, 2024

daogrady commented Dec 19, 2024

daogrady left a comment

daogrady Jan 8, 2025 •

edited

Loading

stockbal Jan 8, 2025

daogrady Jan 8, 2025

daogrady Jan 9, 2025

stockbal Jan 9, 2025

daogrady Jan 9, 2025

daogrady left a comment •

edited

Loading

daogrady commented Jan 9, 2025

stockbal commented Jan 9, 2025

daogrady commented Jan 9, 2025

feat/refactor: dedicated classes for texts and inline comps #356

feat/refactor: dedicated classes for texts and inline comps #356

Conversation

stockbal commented Oct 12, 2024 • edited Loading

Tasks

Sample cds schema

Generated index.ts for namespace bookshop

Generated index.js for namespace bookshop

daogrady commented Oct 23, 2024

daogrady commented Nov 14, 2024

stockbal commented Nov 14, 2024 • edited Loading

daogrady commented Nov 19, 2024

stockbal commented Nov 20, 2024

daogrady commented Nov 26, 2024

stockbal commented Nov 26, 2024

daogrady commented Nov 27, 2024

daogrady commented Dec 4, 2024 • edited Loading

daogrady commented Dec 12, 2024

stockbal commented Dec 12, 2024 • edited Loading

daogrady commented Dec 12, 2024

stockbal commented Dec 12, 2024 • edited Loading

daogrady commented Dec 17, 2024

stockbal commented Dec 17, 2024

daogrady commented Dec 19, 2024

daogrady left a comment

Choose a reason for hiding this comment

daogrady Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

stockbal Jan 8, 2025

Choose a reason for hiding this comment

daogrady Jan 8, 2025

Choose a reason for hiding this comment

daogrady Jan 9, 2025

Choose a reason for hiding this comment

stockbal Jan 9, 2025

Choose a reason for hiding this comment

daogrady Jan 9, 2025

Choose a reason for hiding this comment

daogrady left a comment • edited Loading

Choose a reason for hiding this comment

daogrady commented Jan 9, 2025

stockbal commented Jan 9, 2025

daogrady commented Jan 9, 2025

stockbal commented Oct 12, 2024 •

edited

Loading

Generated `index.ts` for namespace `bookshop`

Generated `index.js` for namespace `bookshop`

stockbal commented Nov 14, 2024 •

edited

Loading

daogrady commented Dec 4, 2024 •

edited

Loading

stockbal commented Dec 12, 2024 •

edited

Loading

stockbal commented Dec 12, 2024 •

edited

Loading

daogrady Jan 8, 2025 •

edited

Loading

daogrady left a comment •

edited

Loading