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.54k stars 4.02k forks source link

Import JDL issue on application update #16420

Closed meistermeier closed 1 year ago

meistermeier commented 3 years ago
Overview of the issue

Re-generating the entities defined in a jdl by invoking jhipster import-jdl entities.jdl with a given entities.jdl https://github.com/meistermeier/jh-13797/blob/d51a6c5c24c374584c6809571187ace055e3cb2e/entities.jdl

entity Exam {
    title String
}

entity Question {
    content String
}

relationship OneToMany {
  Exam{questions} to Question
}

Although the application is neo4j-based, jhipster does not recognize the fact in the relationship creation because configuration.databaseType here: https://github.com/jhipster/generator-jhipster/blob/d72f9852bb691b5ae505f8cbdb0595b08d6d990a/jdl/converters/parsed-jdl-to-jdl-object/parsed-jdl-to-jdl-object-converter.js#L197 is undefined. So there is no information like for the entities where you get the generator.databaseType (or similar(?)) property. Even though supplying the information via an additional application definition in the jdl does not help because in this case jhipster would have to look into configuration.application[0].databaseType. And what if there are multiple applications?

Also if there is no bidirectional mapping of the relationship, the cannot be repositories for both entities. If you would call a delete on a linked Question where the model is not aware of the relationship to Exam. The deletion will fail. All operations have to go through the "aggregate root's" repository, in this case the ExamRepository.

Motivation for or Use Case

This possible bug was discovered while trying to improve the user experience in #13797. Trying to solve this and defining a more sane One-To-Many mapping uncovered this, from my point of view, more fundamental problem. I also manually altered the entities in the linked project to show how they could/should look like for the given jdl.

Reproduce the error

clone https://github.com/meistermeier/jh-13797 branch neo4j-relationships jhipster import-jdl entities.jdl

Related issues

13797

Suggest a Fix

https://github.com/jhipster/generator-jhipster/blob/d72f9852bb691b5ae505f8cbdb0595b08d6d990a/jdl/converters/parsed-jdl-to-jdl-object/parsed-jdl-to-jdl-object-converter.js#L197 needs to be "current" application databaseType aware like the databaseType information in the generator object during entity creation.

JHipster Version(s)

today's (23.09.2021) main branch version

If there is anything missing, I am happy to give you more input on this.

mshima commented 3 years ago

For now try using --unidirectional-relationships flag. IMO we should make this the only behavior for JHipster 8. JDL should pass to the generator exactly what the jdl file defines. Everything else should be business logic defined by the generator.

atomfrede commented 3 years ago

Thanks @mshima for the hint with the flag was not aware of it. Would you agree that the check for databases in the jdl converter can be removed (as it does not work and is at the wrong place) and the check belongs to the respective generators. Are there any objections to allow only unidirectional relationships for neo4j (as these can be derived via the edge between the nodes)?

mshima commented 3 years ago

Would you agree that the check for databases in the jdl converter can be removed (as it does not work and is at the wrong place) and the check belongs to the respective generators.

Yes.

Are there any objections to allow only unidirectional relationships for neo4j?

No.

The problem is that making unidirectional the default behavior is a breaking change to jdl. Blueprints and modules, like ionic, that uses jdl directly may break bad.

Setting default flag value to true at https://github.com/jhipster/generator-jhipster/blob/0194420f2987b75f37984a311f60db57d868d510/cli/commands.js#L119 should be ok to me.

But let’s merge it in after 7.3.0 release to give more time to test.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Our core developers tend to be more verbose on denying. If there is no negative comment, possibly this feature will be accepted. We are accepting PRs :smiley:. Comment or this will be closed in 7 days