cap-js / cds-dbs

Monorepo for SQL Database Services for CAP
https://cap.cloud.sap/docs/
Apache License 2.0
37 stars 11 forks source link

Number Properties become string when service deployed against a HANA database #790

Closed HansenLudwig closed 2 months ago

HansenLudwig commented 3 months ago

Number Properties become string when running the service against a HANA database.

We have an entity defined as follows:

entity Bills : managed {
    key ID     : Integer;
        payer  : Association to one People;
        descr  : localized String(111);
        amount : Decimal;
};

When deployed to BTP again HANA, property amount will be read as type of string, which causes problem at back-end.

"amount": "22.2"

Detailed steps to reproduce

  1. git clone https://github.com/HansenLudwig/CAP-ResReading-TypeError.git
  2. npm install
  3. cds watch
  4. goto .\basic.http file 4.1 send GET requests in the file to the service running on your local, 4.2 and to the service that we've already deployed
    • Or you can simply visit both of the URLs in your browser
  5. Check body of response: Note that the type of the amount attribute returned by the deployed service is a string instead of Number.

This is the response from the locally running service which uses better-sqlite: http://localhost:4004/odata/v4/main-service/Bills(ID=1)

{
  "@odata.context": "$metadata#Bills/$entity",
  //...
  "amount": 11.1
}

When request is sent to the deployed service in the BTP running with a Hana database (db: 'hana'): https://dsc-btp-demo-demo-reading-type-error-srv.cfapps.eu20-001.hana.ondemand.com/odata/v4/main-service/Bills(ID=1)

{
  "@odata.context": "$metadata#Bills/$entity",
  "ID": 1,
  "amount": "11.1",
  //...
}

Details about the demo project

reading_type_error https://github.com/HansenLudwig/CAP-ResReading-TypeError
Node.js v20.12.2
@sap/cds 8.1.1
@sap/cds-compiler 5.1.2
@sap/cds-dk 8.1.2
@cap-js/hana 1.1.1
@cap-js/sqlite 1.7.3
@cap-js/asyncapi 1.0.2
@cap-js/openapi 1.0.5
@sap/cds-fiori 1.2.7
@sap/cds-foss 5.0.1
@sap/cds-mtxs 2.0.5
@sap/eslint-plugin-cds 3.0.4
johannes-vogel commented 3 months ago

I think that this is not really a database issue but related to the new odata protocol adapter that's part of cds8.

Background:

The difference between SQLite and SAP HANA results was always there as SAP HANA clients protect you from rounding problems while SQLite driver doesn't. Imagine a decimal greater than the one Javascript can safely represent.

This always has caused a rounding issue in the server.


The new odata adapter does not differentiate on the presence of IEEE754Compatible format parameter but always returns the format as is from the server. The old protocol adapter did convert to JSON numbers if format parameter was absent causing precision loss.


To overcome that, it was decided to always return the format as is and clients should be able to work with string representation anyway.

We do have a new configuration that makes SQLite look like SAP HANA even though rounding issues on the database itself still can occur due to nature of SQLite.

Hope that made sense!

fabiantschirschnitz commented 2 months ago

Dear @johannes-vogel,

thanks for your comprehensive explanation about this issue. Especially for pointing out the feature toggle to experience same results at dev- and runtime (sqlite vs. hana). I got you right, that the advice is to treat responses with Number fields from cds always as strings properties though? My only concern is that e.g. if you use cds-typer, the tsc compiler will complain about those lines:

...
import {Bill, Bills} from '@cds-models/db'
const bill : Bill =  await SELECT.from(Bills) //>
//@ts-expect-error <-- you have to ignore the warning about bill.amount being Number
const baseAmount =  parseFloat(bill.amount),
...

Is there any good recommendation how to nicely still be able to work with cds-typer classes? Should we open an Issue to cds-typer, to also set | string as a datatype for numerical fields?

Thanks for your patience, Fabian.

johannes-vogel commented 2 months ago

Hi @fabiantschirschnitz,

yes, I think that has to be adapted in cds-typer as otherwise precision might be lost.

Best, Johannes

patricebender commented 2 months ago

please open an issue in the cds-typer repository if you wish to follow up on that