pofider / node-odata-to-sql

Generate SQL from OData in node.js
MIT License
5 stars 4 forks source link

Inappropriate varchar2(4000) type for Edm.String at case of oracle #4

Open SlavaUtesinov opened 3 years ago

SlavaUtesinov commented 3 years ago

I created on my own draft of jsreport-oracle-store and when I tried to test it with jsreport I immediately faced a problem at stage of creating sample templates ("sample-template": { "createSamples": true }). The reason is a type selected for Edm.String at case of oracle database is varchar2(4000) (oracle.js). I know, it is restriction of oracle, but on the other hand it is obviously not enough (for example, you use varchar(max) for mssql) even for your own templates. Probably you should use CLOB type for unlimited string content, but at this case you also have to construct another sql scripts for them, because scripts like where clobColumnName = 'value' is not supported for CLOB, moreover possibly some castings to and from CLOB type also have to be done.

module.exports = {
  'Edm.String': 'varchar2(4000)',
  'Edm.DateTimeOffset': 'timestamp',
  'Edm.Boolean': 'number(1)',
  'Edm.Int32': 'number',
  'Edm.Int64': 'number',
  'Edm.Decimal': 'number(18,4)',
  'Edm.Binary': 'clob'
}
pofider commented 3 years ago

Thank you for your analysis, you found a valid need.

Please let me know if you are missing something to continue further. Thank you for your work on oracle-store.

jorisdebock commented 3 years ago

@pofider

I have added the following to the oracle-store pull request: https://github.com/jsreport/jsreport-oracle-store/pull/1/commits/8f08011593a4e4cb482a85bf53c9f83d22edc739

Is this the way to go?

Is this also correct behavior with binary type, since its also a clob? Can you point me to a plugin where binary is used so I can test it?

jorisdebock commented 3 years ago

The following type should also use clob:

jorisdebock commented 3 years ago

@pofider instead of using maxLength, is it maybe better to define a new type like "Edm.StringMax"?

Currently in oracle-store-extension there is now this:

if (propDef.type === 'Edm.String' && propDef.document) {
  propDef.maxLength = 'max'
} else if (propDef.type === 'Edm.String' && typeName === 'SettingType' && propName === 'value') {
  propDef.maxLength = 'max'
}

I think its more maintainable and less error prone to use the Edm.StringMax when defining the model