[BUG] Duplicate identifier in generated code

mcastrup commented 5 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Nature of Your Project

TypeScript

Current Behavior

We defiend an entity Transactions in our model:

entity Transactions                    as(
    select from WalletTransactions {
        key ID,
            displayId,
            Type,
            referenceObjectId,
            ReferenceEntityType,
            hash,
            nonce,
            Direction,
            Wallet,
            Asset,
            fromAddress,
            toAddress,
            value,
            Asset.Cryptocurrency as ValueCurrency,
            actualNetworkFee     as networkFee,
            NetworkFeeCurrency,
            outboxedOn,
            broadcastOn,
            confirmedOn,
            'Confirmed'          as Status_code : String, // this field needs to be defined explicitly, as it is not generated by the association
            Status                              : Association to codelists.OutboxedTransactionStatusCodes
                                                      on Status.code = 'Confirmed'
    }
)
union all
(
    select from OutboxedTransactions {
        ID,
        displayId,
        Type,
        referenceObjectId,
        ReferenceEntityType,
        hash,
        nonce,
        'Outgoing'           as Direction_code,
        Wallet,
        Asset,
        fromAddress,
        toAddress,
        value,
        Asset.Cryptocurrency as ValueCurrency,
        authorizedNetworkFee as networkFee,
        NetworkFeeCurrency,
        createdAt            as outboxedOn,
        broadcastOn,
        confirmedOn,
        Status
    }
);

The whole file is available here.

When running cds-typer, the following code is created in the index.ts file;

export function _TransactionAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class Transaction extends Base {
        ID?: string;
        displayId?: string | null;
        Type?: __.Association.to<_sap_erp4sme_c4b_core_wallets_codelists.TransactionTypeCode> | null;
        Type_code?: string | null;
        referenceObjectId?: string | null;
        ReferenceEntityType?: __.Association.to<_sap_erp4sme_c4b_common_codelists.EntityTypeCode> | null;
        ReferenceEntityType_code?: string | null;
        hash?: string | null;
        nonce?: number | null;
        Direction?: __.Association.to<_sap_erp4sme_c4b_core_wallets_codelists.TransactionDirectionCode> | null;
        Direction_code?: string | null;
        Wallet?: __.Association.to<Wallet> | null;
        Wallet_ID?: string | null;
        Asset?: __.Association.to<WalletAsset> | null;
        Asset_ID?: string | null;
        fromAddress?: string | null;
        toAddress?: string | null;
        value?: number | null;
        ValueCurrency?: __.Association.to<_sap_erp4sme_c4b_core_cryptocurrencies.Cryptocurrency> | null;
        networkFee?: number | null;
        NetworkFeeCurrency?: __.Association.to<_sap_erp4sme_c4b_core_cryptocurrencies.Cryptocurrency> | null;
        outboxedOn?: string | null;
        broadcastOn?: string | null;
        confirmedOn?: string | null;
        Status_code?: string | null;
        Status?: __.Association.to<_sap_erp4sme_c4b_core_wallets_codelists.OutboxedTransactionStatusCode> | null;
        Status_code?: string | null;
      static actions: {
    }
  };
}

The property Status_code appears twice, which leads to the error: "Duplicate identifier 'Status_code'."

Expected Behavior

I expect the last property Status_code not to be generated, like:

export function _TransactionAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class Transaction extends Base {
        ID?: string;
        displayId?: string | null;
        Type?: __.Association.to<_sap_erp4sme_c4b_core_wallets_codelists.TransactionTypeCode> | null;
        Type_code?: string | null;
        referenceObjectId?: string | null;
        ReferenceEntityType?: __.Association.to<_sap_erp4sme_c4b_common_codelists.EntityTypeCode> | null;
        ReferenceEntityType_code?: string | null;
        hash?: string | null;
        nonce?: number | null;
        Direction?: __.Association.to<_sap_erp4sme_c4b_core_wallets_codelists.TransactionDirectionCode> | null;
        Direction_code?: string | null;
        Wallet?: __.Association.to<Wallet> | null;
        Wallet_ID?: string | null;
        Asset?: __.Association.to<WalletAsset> | null;
        Asset_ID?: string | null;
        fromAddress?: string | null;
        toAddress?: string | null;
        value?: number | null;
        ValueCurrency?: __.Association.to<_sap_erp4sme_c4b_core_cryptocurrencies.Cryptocurrency> | null;
        networkFee?: number | null;
        NetworkFeeCurrency?: __.Association.to<_sap_erp4sme_c4b_core_cryptocurrencies.Cryptocurrency> | null;
        outboxedOn?: string | null;
        broadcastOn?: string | null;
        confirmedOn?: string | null;
        Status_code?: string | null;
        Status?: __.Association.to<_sap_erp4sme_c4b_core_wallets_codelists.OutboxedTransactionStatusCode> | null;
      static actions: {
    }
  };
}

When I compare the cds-typer generation with that of cds2types, I see the following result, created by cds2types:

export interface Transactions {
        ID: string;
        displayId?: string;
        Type?: TransactionTypeCodes;
        Type_code?: string;
        referenceObjectId?: string;
        ReferenceEntityType?: sap.erp4sme.c4b.common.codelists.EntityTypeCodes;
        ReferenceEntityType_code?: string;
        hash?: string;
        nonce?: number;
        Direction?: TransactionDirectionCodes;
        Direction_code?: string;
        Wallet?: Wallets;
        Wallet_ID?: string;
        Asset?: WalletAssets;
        Asset_ID?: string;
        fromAddress?: string;
        toAddress?: string;
        value?: BigSource;
        ValueCurrency?: sap.erp4sme.c4b.core.cryptocurrencies.Cryptocurrencies;
        ValueCurrency_ID?: string;
        networkFee?: BigSource;
        NetworkFeeCurrency?: sap.erp4sme.c4b.core.cryptocurrencies.Cryptocurrencies;
        NetworkFeeCurrency_ID?: string;
        outboxedOn?: Date;
        broadcastOn?: Date;
        confirmedOn?: Date;
        Status_code?: string;
        Status?: OutboxedTransactionStatusCodes;
    }

Here, the Status_code appears only once.

Steps To Reproduce

You can clone our repository crypto-for-business locally with git clone https://github.tools.sap/erp4sme/crypto-for-business.git, checkout the branch switch_to_typer_prototype and run npm ci and npm run cds-typer:dev

Environment

- **OS**: Windows
- **Node**: v18.13.0
- **npm**: 8.19.3
- **cds-typer**: 0.15.0
- **cds**: 7.4.1

Repository Containing a Minimal Reproducible Example

No response

Anything else?

There is already a similar issue: https://github.com/cap-js/cds-typer/issues/139 I think, it is only similar, but not the same.

hakimio commented 5 months ago

The reason for this duplication is in your definition here:

The Status association results in Status_code field. 'Confirmed' as Status_code : String, is redundant here.

I think the issue is with your definition, not cds-typer. Status_code should be defined automatically from the association, not sure why it's not for you.

daogrady commented 5 months ago

Hi Michael,

thank you for the detailed report, and thank you, Tomas, for looking into the issue! I agree with Tomas' observation. Still, I also acknowledge the potential for confusion if manually defined fields clash with ones that have been generated automatically -- regardless of the reason.

I will therefore address this issue by raising an error and not generating the foreign key field in that case.

Best, Daniel

mcastrup commented 5 months ago

Hi @daogrady ,

I have still two questions regarding your and Tomas' answers:

Why do you include the key field of the associated entity into the generated artefacts, i.e. in this case the code property of OutboxedTransactionStatusCodes as Status_code?
We are trying to replace the module cds2types by your cds-typer. As I already mentioned, the type, which is generated by cds2types, includes only the Status property from the association. The property Status_code in the generated type is related to the explicitly defined property in the entity.
I also played around and tried several options to refactor the entity. However, I think there is a reason for the comment in the code:
'Confirmed' as Status_code : String, // this field needs to be defined explicitly, as it is not generated by the association
I think cds-typer should be able to handle such field clashes regardless of the reason, as you also mentioned.
You mention to raise an error and not to generate the foreign key field in such a case. Does this mean, the whole generation of all the types will break? Or dou you just ignore the duplicate property and everything else will work fine?

Best regards, Michael

daogrady commented 5 months ago

Hi Michael,

I am not an expert CDS user myself, so I have to reply to this to the best of my knowledge: when defining an association from an entity A to an entity B, where B has fields that are declared as key, you can reference these fields via an underscore. This is apparently called "foreign key propagation" and I suppose it is done to avoid loading endless association chains when you just need the top level entity. This feature goes back to cds-typer v0.8, as it had been requested several times internally and here on GitHub. You can see one such issue here.

I think cds-typer should be able to handle such field clashes regardless of the reason, as you also mentioned.

I am not sure how such a handling would look like. The workaround I will employ now is to not generate a foreign key field if the entity in question owns a property of the exact name the FK would have. This will avoid clashes. But it will inevitably lead to problems when a user tries to use such a field with the foreign key semantic, when in fact it is just a property that happens to have the same name. Therefore the error message.

cds-typer will run to completion with the behaviour mentioned above. I advise strongly to consider a different name for the offending field, if possible.

Best, Daniel

cap-js / cds-typer