#671: add support for hana as dbType #671
Conversation
| CREATE TABLE BinaryObject ( | ||
| id BIGINT NOT NULL GENERATED BY DEFAULT AS IDENTITY, | ||
| modificationCounter INTEGER NOT NULL, | ||
| content CLOB, |
There was a problem hiding this comment.
OK. I will fix that and hope it works with @lob annotation and hibernate validation.
| -- *** RevInfo (Commit log for envers audit trail) *** | ||
| CREATE COLUMN TABLE RevInfo( | ||
| id BIGINT NOT NULL GENERATED BY DEFAULT AS IDENTITY (START WITH 1), | ||
| "timestamp" BIGINT NOT NULL, |
There was a problem hiding this comment.
Unquoted columns will be upper-cased while quoted columns will be mixed-case. So for consistency reasons you should name this column "TIMESTAMP".
There was a problem hiding this comment.
The reason here is that we had to quote this property also in the JPA annotation as this is a reserved keyword in Oracle.
https://github.com/oasp/oasp4j/blob/develop/modules/jpa-basic/src/main/java/io/oasp/module/jpa/dataaccess/api/AdvancedRevisionEntity.java#L41
For some reasons (case-sensitivity) in DDL for most databases this property then also has to be quoted in the same way. To keep this consistent we do it the same way for the DDLs of all database types.
There was a problem hiding this comment.
BTW: Using "TIMESTAMP" in AdvancedRevisionEntity.java would indeed have been the better option for 2.6.x. Changing this again? Or leave it as is?
| content CLOB, | ||
| filesize BIGINT NOT NULL, | ||
| mimeType VARCHAR(255), | ||
| CONSTRAINT PK_BinaryObject_id PRIMARY KEY(ID) |
There was a problem hiding this comment.
This could just be PRIMARY KEY(ID) if you don't need an explicit name.
There was a problem hiding this comment.
Thing was that at least in Oracle we often get constraint violations that do not tell the context in the error message. However, we always get the name of the constraint in the error message. If no name is given, then an autogenerated name will be choose what is kind of random and hence you still do not know what is wrong in case of an error.
Therefore we defined strict naming conventions as best practices for all constraints:
https://github.com/oasp/oasp4j/wiki/guide-sql#ddl
In general you are right that functionally this is just clutter.
| @@ -0,0 +1,9 @@ | |||
| -- *** BinaryObject (BLOBs) *** | |||
| CREATE TABLE BinaryObject ( | |||
There was a problem hiding this comment.
Good finding. I will fix that asap.
There was a problem hiding this comment.
Was able to run the application successfully with SAP Hana database. Tables are getting generated as expected.
An exception is thrown during the application startup: "com.sap.db.jdbc.exceptions.SQLFeatureNotSupportedExceptionSapDB: Method createClob() of Connection is not supported.". Reading on the forums(https://stackoverflow.com/a/4592863/4057038) it seems to be due to Hibernate checks on startup and might not cause any real problems(but not sure). But I think we can move this to bug fix release. Once the changes are done this PR can be merged.
| @@ -39,6 +39,10 @@ spring.datasource.username=${rootArtifactId} | |||
| # spring.jpa.database-platform=org.hibernate.dialect.MySQL5Dialect | |||
| # spring.datasource.driver-class-name=org.mariadb.jdbc.Driver | |||
| spring.datasource.username=${rootArtifactId} | |||
| #elseif ($dbType == 'hana') | |||
There was a problem hiding this comment.
Property spring.jpa.database is set to hana, which is not a valid value. As of now hana database cannot be set explicitly using this property. So from the list of available values only DEFAULT seems to be a sensible choice and using it Hibernate is able to resolve the database correctly. So it is better to set this property to DEFAULT
There was a problem hiding this comment.
Setting HANA via spring.jpa.database will be possible starting with Spring 5.1 (see Jira item https://jira.spring.io/browse/SPR-16460)
There was a problem hiding this comment.
Thanks for all your valuable feedback. Also great seeing how hana evolves and becomes first class citizen by all frameworks 👍
With this information, I would suggest not to change anything. We just leave it like this and it will get fixed in the future when the update to spring 5.1 takes place.
There was a problem hiding this comment.
@hohwille but with the current setting application fails to startup. So until Spring 5.1 is available we need to provide a valid value that works for Hana.
| @@ -27,6 +27,9 @@ spring.datasource.url=jdbc:mysql://address=(protocol=tcp)(host=localhost)(port=3 | |||
| #elseif ($dbType == 'mariadb') | |||
| spring.datasource.password=todo | |||
| spring.datasource.url=jdbc:mariadb://localhost:3306/db | |||
| #elseif ($dbType == 'hana') | |||
| spring.datasource.password=todo | |||
| spring.datasource.url=jdbc:sap://localhost:39015 | |||
There was a problem hiding this comment.
As explained here, there is variation in port number as per the version of database. I would suggest not to hard code the port number and let the user figure out what port number to use. Otherwise chances are that users(especially junior members of the team) will be confused why he/she is getting connection error without realizing that the port number configured is not correct for their setup.
There was a problem hiding this comment.
39015 is the default port of the HANA express cloud and VM images. IMHO this is a sensible default. I'd just add a comment how to adapt it for other HANA installations.
There was a problem hiding this comment.
Thanks also for this information. We intentionally configure this here as this is the configuration to be used by developers in the development environment. This config file will not be included into the project result artifact (WAR or JAR). However, all developers should have a common setup for all their machines to simplify. When the default port number is not suitable for a project, it can be easily changed after the project template has been instantiated.
@breglerj Do you have a recommended link to a guide how to adapt it for other HANA installations? I would add this as a comment then.
There was a problem hiding this comment.
@hohwille: Here is a link to the client documentation that describes which port to use: https://help.sap.com/viewer/0eec0d68141541d1b07893a39944924e/latest/en-US/b250e7fef8614ea0a0973d58eb73bda8.html
|
The "com.sap.db.jdbc.exceptions.SQLFeatureNotSupportedExceptionSapDB: Method createClob() of Connection is not supported." error is indeed just Hibernate checking the driver for features and can be ignored. |
|
I was on vacation. Seems that this has blocked the entire release. Lets be a little bit more pragmatic:
|
After we added hana support to MTS via devonfw/my-thai-star#94 we also want to have hana support for our application template (maven archtetype). This is provided by this PR.