feat: Respect precision for Odata V4 type Edm.DateTimeOffset
#5361
Conversation
KavithaSiva
changed the title
Edm.DateTimeOffsetEdm.DateTimeOffset
| '@sap-cloud-sdk/generator': minor | ||
| '@sap-cloud-sdk/odata-v4': minor | ||
| '@sap-cloud-sdk/odata-common': minor |
There was a problem hiding this comment.
[q] Should I write major here? What is the correct heading for the PR then? feat needs a changeset with minor.
| const serialize = deSerializers[edmType as any]?.serialize; | ||
| // Check if the edmType is specifically Edm.DateTimeOffset as we can have precision, scale property defined | ||
| if (edmType === 'Edm.DateTimeOffset' && precision) { |
There was a problem hiding this comment.
[req] If precision is 0 (which is a valid value) this will be falsy.
[q] Can we double check if it's only DateTimeOffset? or also valid for TimeStamp?
[q] Also, even if we just send precision without any checks, does it break anything? With JS it just ignores the extra params.
| @@ -14,6 +14,7 @@ import { | |||
| serializeToTime, | |||
| deserializeDateToMoment | |||
| } from './converters'; | |||
| import type { Moment } from 'moment'; | |||
There was a problem hiding this comment.
I think this is not the correct way to import moment.
There was a problem hiding this comment.
I now removed the import as moment seems to be accessible without it in this file.
But, I don't understand how this works, and why we explicitly use import { Moment } from 'moment' in some places but use it directly in some.
There was a problem hiding this comment.
I think it's likely due to moment being a global namespace here (which allows using it without import statement). But for consistency we should still import it.
There was a problem hiding this comment.
If you're using moment only for its type then use:
import type moment else import moment
| @@ -603,7 +605,8 @@ export class TestEntity50PropApi< | |||
| DECIMAL_PROPERTY_2: fieldBuilder.buildEdmTypeField( | |||
| 'DecimalProperty2', | |||
| 'Edm.Decimal', | |||
| true | |||
| true, | |||
| 9 | |||
There was a problem hiding this comment.
[q] What is the max allowed precision as per odata? Just want to make sure we're testing the correct values. If I remember cds allows only till 7
There was a problem hiding this comment.
From OData spec:
For a decimal value: the maximum number of significant decimal digits of the property’s value; it MUST be
a positive integer.
For a temporal value (datetime-with-timezone-offset, duration, or time-of-day): the number of decimal
places allowed in the seconds portion of the value; it MUST be a non-negative integer between zero and
twelve.
Currently, precision values get generated for both temporal and decimal values.
But, I only use it for formatting DateTimeOffset fields.
I am okay with generating them for decimal fields as well, eventhough we don't use it, do you feel otherwise?
There was a problem hiding this comment.
I think it would be nicer if we generate it for types where we actually support it. otherwise it might be confusing.
There was a problem hiding this comment.
I would only update our test client code for cases where we actually support the behavior.
There was a problem hiding this comment.
It was updated automatically now during generation, as we always add precision if it's available in the EDMX file.
Let me check, how to restrict this during generation only for temporal fields.
test-packages/test-services-odata-v4/test-service/TestEntityApi.ts
Outdated
Show resolved
Hide resolved
Closes SAP/cloud-sdk-backlog#1261.
v3-mainnow, which is not for v4 development.mainbranch (for v4), I've updated (V4-Upgrade-Guide.md)[./V4-Upgrade-Guide.md] in case my change has any implications for users updating to SDK v4