jhipster / generator-jhipster

JHipster is a development platform to quickly generate, develop, & deploy modern web applications & microservice architectures.
https://www.jhipster.tech
Apache License 2.0
21.44k stars 4.02k forks source link

UUID id fields generate autoincrement attribute which fails application startup #17835

Closed OmarHawk closed 2 years ago

OmarHawk commented 2 years ago
Overview of the issue

For id fields having UUID type, the generator generates autoIncrement="true" in my configuration, even though during startup of such an application, an error is being produced:

This is produced in the liquibase entity changelog for adding the field

            <column name="uuid" type="${uuidType}" autoIncrement="true">
                <constraints primaryKey="true" nullable="false"/>
            </column>

even though in master.xml a varchar field is produced.

    <property name="uuidType" value="varchar(36)" dbms="h2, mysql, mariadb"/>

So during startup, such an error is produced:

org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'liquibase' defined in class path resource [de/mt/dataview/config/LiquibaseConfiguration.class]: Invocation of init method failed; nested exception is liquibase.exception.LiquibaseException: liquibase.exception.UnexpectedLiquibaseException: Property [autoIncrement] was not found for object type [liquibase.datatype.core.VarcharType]
Motivation for or Use Case

Application will not start up.

Reproduce the error
Related issues

17833

Suggest a Fix

I think, it is related to the reactive branch in this if-else statement:

https://github.com/jhipster/generator-jhipster/blob/2dc5a892a8fbf216f78372058974c663078c822f/utils/field.js#L279-L284

where the liquibaseAutoIncrement value is statically set to true. Seems to be just not considered in d63b9d695573433977473e6503df7e7fc14704f3

JHipster Version(s)

7.6.0 (latest SNAPSHOT now, since other bugs in the last release cover up this one, e.g. #17833, #17823)

JHipster configuration
JHipster Version(s)
dataview@0.0.1-SNAPSHOT C:\Users\jwedding\git\mt\demo3
`-- (empty)
JHipster configuration, a .yo-rc.json file generated in the root folder
.yo-rc.json file
{
  "generator-jhipster": {
    "applicationType": "monolith",
    "authenticationType": "oauth2",
    "baseName": "dataview",
    "blueprints": [],
    "buildTool": "maven",
    "cacheProvider": "no",
    "clientFramework": "react",
    "clientPackageManager": "npm",
    "clientTheme": "none",
    "clientThemeVariant": "",
    "creationTimestamp": 1639736978010,
    "databaseType": "sql",
    "devDatabaseType": "mysql",
    "devServerPort": 9060,
    "dtoSuffix": "DTO",
    "enableGradleEnterprise": false,
    "enableHibernateCache": false,
    "enableSwaggerCodegen": false,
    "enableTranslation": true,
    "entities": ["DataStream"],
    "entitySuffix": "",
    "incrementalChangelog": true,
    "jhiPrefix": "jhi",
    "jhipsterVersion": "7.6.0",
    "languages": ["de", "en"],
    "lastLiquibaseTimestamp": 1644410794000,
    "messageBroker": false,
    "nativeLanguage": "de",
    "otherModules": [],
    "packageName": "de.mt.dataview",
    "pages": [],
    "prodDatabaseType": "mysql",
    "reactive": true,
    "rememberMeKey": "YourJWTSecretKeyWasReplacedByThisMeaninglessTextByTheJHipsterInfoCommandForObviousSecurityReasons",
    "searchEngine": false,
    "serverPort": "8080",
    "serverSideOptions": [],
    "serviceDiscoveryType": "no",
    "skipCheckLengthOfIdentifier": false,
    "skipFakeData": false,
    "skipJhipsterDependencies": true,
    "skipUserManagement": true,
    "testFrameworks": [],
    "websocket": false,
    "withAdminUi": true
  }
}

JDL for the Entity configuration(s) entityName.json files generated in the .jhipster directory
JDL entity definitions
entity DataStream {
  @Id uuid UUID
}
paginate DataStream with pagination
service DataStream with serviceImpl

Environment and Tools

openjdk version "11.0.12" 2021-07-20 LTS OpenJDK Runtime Environment 18.9 (build 11.0.12+7-LTS) OpenJDK 64-Bit Server VM 18.9 (build 11.0.12+7-LTS, mixed mode)

git version 2.35.1.windows.2

node: v14.18.2

npm: 6.14.15

Docker version 20.10.9, build c2ea9bc

No change to package.json was detected. No package manager install will be executed. Congratulations, JHipster execution is complete!

Entity configuration(s) entityName.json files generated in the .jhipster directory
Browsers and Operating System
OmarHawk commented 2 years ago

@mshima - since you are the original owner of related parts of the code, maybe you can give some hints to me how to fix it (or if more efficient fix it by yourself)?

I'll try suggesting something though via some pull request, but I think, this may then really require some help this time, since I may not foresee certain variants out there.

mshima commented 2 years ago

This may work:

    } else if (entityWithConfig.reactive) {
      const autoincrement = LONG === field.fieldType;
      field.liquibaseAutoIncrement = autoincrement;
      field.jpaGeneratedValue = false;
      field.autoGenerateByService = !autoincrement;
      field.autoGenerateByRepository = autoincrement;
      field.readonly = true;
OmarHawk commented 2 years ago

crazy: I did the very same in my local branch :D (and am currently testing it out) ;)

mshima commented 2 years ago

My initial implementation was focused on the internal implementation using defaults (imperative + angular only). As you saw the other PR, react support was added later by another colaborator.

You are implementing webflux + custom-ids + react. You may want to create webflux-react-adittional test to avoid regressions, like: https://github.com/jhipster/generator-jhipster/blob/89a022b9ec1784f2edac26197a100e314cad4fcc/.github/workflows/angular.yml#L104-L112

with jdl-entity: 'custom-id.jdl' at: https://github.com/jhipster/generator-jhipster/blob/main/.github/workflows/webflux.yml

OmarHawk commented 2 years ago

Ok, will not be enough for UUID (or other types, except now the Long type). It will result in an error when saving a new entity, like this:

org.springframework.dao.TransientDataAccessResourceException: Failed to update table [data_stream]. Row with Id [530e6d0d-7860-4547-a1bd-df4ab705a265] does not exist.

This is because we probably have to generate the Entity with implementation of the Persistable interface (reference https://docs.spring.io/spring-data/jdbc/docs/current/reference/html/#is-new-state-detection ) to allow to mark an entity as new... I'll try to include this in my Pull Request then. :-)

This may then also resolve #14569, as also suggested in https://github.com/jhipster/generator-jhipster/issues/14569#issuecomment-828419358

OmarHawk commented 2 years ago

And for sure, I'll try to add some test case(s) 👍

OmarHawk commented 2 years ago

Just to give some status update: I managed to get the Entity saving, but javax's @PostLoad and @PostPersist are not a thing in the reactive world, so I will have to go for https://docs.spring.io/spring-data/r2dbc/docs/current/reference/html/#r2dbc.entity-callbacks

mshima commented 2 years ago

Are those callbacks needed? Why don’t set isNew to true together with the id?

OmarHawk commented 2 years ago

Are those callbacks needed? Why don’t set isNew to true together with the id?

well, I ended up doing it like that as well, as for full updates the entity is not loaded from database in the first place, therefore never getting the callback being called. I just wonder, if the same is true for partial updates or anything else, like some developer actually doing own business logic with entities from the database and modifying stuff there.

OmarHawk commented 2 years ago

with jdl-entity: 'custom-id.jdl'

@mshima , I think, this would only work if there would be such a file named like this in https://github.com/jhipster/jdl-samples See https://github.com/jhipster/generator-jhipster/runs/5274599189?check_suite_focus=true#step:13:53 when there is a star instead. So it expects files to be present in this repo, if I name field jdl-entity (in contrast to "entity", where it takes it from the local repo I guess)

OmarHawk commented 2 years ago

Are those callbacks needed? Why don’t set isNew to true together with the id?

I just found out, that they are @mshima (and therefore some IT tests are failing at the moment). Spring Data uses this internally to check, if it can delete an entity that is referenced by ID:

grafik

I'll check, if there is a way to generate this kind of thing with AOP instead of having actual classes (as I already had in).

mshima commented 2 years ago

@OmarHawk probably you should implement setIsNew instead of setIsPersisted. This will set the default state as persisted and should fix the delete issue.

OmarHawk commented 2 years ago

@mshima I just tried that, but seems like some internal cache then causes the isNew to be set to true. The previously saved element by the entity manager within the IT, where I have to set the Entity to new in the first place as well (which is not a good thing from a developers perspective as I need to know too much about the lifecycle of Entities depending on my chosen framework!) is not fetched again from database!

grafik

So the same test is still failing.

To be honest, I think, we should just go the way the official Spring documentation advises us instead of producing crappy workarounds and wasting even more time.

Here a branch so that you may believe, I actually did it as you suggested, but its turning out to be a bad idea: https://github.com/OmarHawk/generator-jhipster/commit/7b7d72da7b645679de7c41259c67160bfcdd697f

OmarHawk commented 2 years ago

Meanwhile I also added also support for the non-reactive stack and made all tests green. I now do generate the Persistable Interface and added support for the reactive & non-reactive stack and actually implemented it the way official documentation suggests (and as I initially also suggested). Sure, we do have one additional class in the reactive stack, but this is more understandable and less error prone + more maintainable than messing around with AOP within the Spring framework, trying to implement the same mechanism by ourselves.

mraible commented 2 years ago

Adding a bug bounty because I'm a big fan of UUID after experiencing a painful production experience with PostgreSQL. https://github.com/mraible/21-points/pull/58

OmarHawk commented 2 years ago

@mraible: Well, the linked PR will fix that - but please note the last comment in there that the currently failing test is due to a bug in spring-data-relational and we probably won't get it fixed till the next Spring Boot Version (2.7.x), which will bring the required bugfix transitively into the reactive area. ;-)

hubertgrzeskowiak commented 2 years ago

For those using the latest stable and trying to use UUIDs as IDs, I manually changed the

autoIncrement="true"

to

autoIncrement="false"

in each change log before running the Spring Boot app (which applies Liquibase changes), and it seems to work just fine.