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

Implement nullable fields for compiled TS types #108

Closed

Conversation

siarhei-murkou
Copy link
Contributor

@siarhei-murkou siarhei-murkou commented Nov 14, 2023

Description:

This feature allows to compile nullable CDS fields to nullable TypeScript properties using ...: ... | null construction.

Example:

entity Obj1 {
    name1: String not null;
    name2: String;

    obj1: Association to one Obj1 not null;
    obj2: Association to one Obj2;
}

This CDS entity is compiled to

class Obj1 {
    name1: string;
    name2: string | null;
    
    obj1_ID: string;
    obj1: Association<Obj1>;
    
    obj2_ID: string | null;
    obj2: Association<Obj2> | null;
}

Additionally, this feature works with managed Associations if to mark managed association with "not null".

@siarhei-murkou siarhei-murkou marked this pull request as draft November 14, 2023 16:58
@siarhei-murkou siarhei-murkou force-pushed the feature/nullable-cds-fields branch from e6678d5 to 32be0d3 Compare November 15, 2023 10:56
@siarhei-murkou siarhei-murkou force-pushed the feature/nullable-cds-fields branch from bc31dd8 to 6b22262 Compare November 15, 2023 11:57
@siarhei-murkou siarhei-murkou marked this pull request as ready for review November 15, 2023 12:06
Copy link
Contributor

@daogrady daogrady left a comment

Choose a reason for hiding this comment

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

I am currently checking your changes. They look pretty good so far, but I'll give it another round of testing in a bit.

Generally, could you please revert the formatting changes you have applied to the .cds files in the test directory?
I am absolutely open to changing their formatting in a second PR, but it bloats the PR at hand unnecessarily.

@daogrady
Copy link
Contributor

daogrady commented Nov 20, 2023

I just checked the output of some of the tests. I don't think there is really any explicit testing going on as of now. You did add the not null suffix to many test models, but you did not check whether this was actually reflected in the generated files.

I have found that for the following model:

// model.cds
entity Foo {}

entity Bar {
    a : { b : Association to one Foo not null; } not null;
};

we will currently receive the following generated file:

// This is an automatically generated file. Please do not change its contents manually!
import * as __ from './_';
export function _FooAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class Foo extends Base {
      static actions: {
    }
  };
}
export class Foo extends _FooAspect(__.Entity) {}
export class Foo_ extends Array<Foo> {}
Object.defineProperty(Foo, 'name', { value: 'Foo' })
Object.defineProperty(Foo_, 'name', { value: 'Foo' })

export function _BarAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class Bar extends Base {
        a?: {
      b?: __.Association.to<Foo>,
    };
      static actions: {
    }
  };
}
export class Bar extends _BarAspect(__.Entity) {}
export class Bar_ extends Array<Bar> {}
Object.defineProperty(Bar, 'name', { value: 'Bar' })
Object.defineProperty(Bar_, 'name', { value: 'Bar' })

which is lacking the | null type in the relevant parts.

Admittedly, writing the tests is probably the most cumbersome part of extending cds-typer and it is not at all documented right now how to do it properly. So I will extend on that part of the documentation in the future.

Copy link
Contributor

@daogrady daogrady left a comment

Choose a reason for hiding this comment

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

At least nested types don't reflect the not null flag properly.
Test cases need to be checked explicitly for the presence of the null type. Plus, we should have parts of the model that do and do not have not null. Adding it everywhere will likely blind us for types that are explicitly nullable.

@siarhei-murkou siarhei-murkou force-pushed the feature/nullable-cds-fields branch from 524ae1b to 160d261 Compare November 20, 2023 11:56
@daogrady
Copy link
Contributor

This PR has been superseded by #111 in which I have made the adjustments we talked about, so I am closing this one.

@daogrady daogrady closed this Nov 23, 2023
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