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

Move DTO option from entity to global #5227

Closed nicoraynaud closed 7 years ago

nicoraynaud commented 7 years ago
Overview of the issue

I have created a jdl file to reproduce the issue : jhipster-jdl.txt

The link between Line and Category is not properly done in LineDTO. It creates a Set of CategoryDTO but Category does not have a DTO representing it.

Motivation for or Use Case

The DTO should link to the entity if there is no DTO representing it.

Reproduce the error

Create a new project, with the following domain : jhipster-jdl.txt

Suggest a Fix
JHipster Version(s)
easycount@0.0.0 F:\workspace\easycount
`-- generator-jhipster@4.0.3  -> C:\Dev\jhipster\generator-jhipster
  `-- generator-jhipster@4.0.3  extraneous
JHipster configuration, a .yo-rc.json file generated in the root folder
{
  "generator-jhipster": {
    "jhipsterVersion": "4.0.3",
    "baseName": "easycount",
    "packageName": "org.tekinico.easycount",
    "packageFolder": "org/tekinico/easycount",
    "serverPort": "8080",
    "authenticationType": "jwt",
    "hibernateCache": "ehcache",
    "clusteredHttpSession": false,
    "websocket": false,
    "databaseType": "sql",
    "devDatabaseType": "postgresql",
    "prodDatabaseType": "postgresql",
    "searchEngine": false,
    "messageBroker": false,
    "serviceDiscoveryType": false,
    "buildTool": "maven",
    "enableSocialSignIn": false,
    "jwtSecretKey": "7ce176028b1c46a004dcaaf7c3cca4391e809d1c",
    "useSass": true,
    "clientPackageManager": "yarn",
    "applicationType": "monolith",
    "clientFramework": "angular2",
    "testFrameworks": [
      "gatling"
    ],
    "jhiPrefix": "jhi",
    "enableTranslation": true,
    "nativeLanguage": "fr",
    "languages": [
      "fr",
      "en"
    ]
  }
}
Entity configuration(s) entityName.json files generated in the .jhipster directory

See the enclosed jdl file : jhipster-jdl.txt

Browsers and Operating System

java version "1.8.0_112" Java(TM) SE Runtime Environment (build 1.8.0_112-b15) Java HotSpot(TM) 64-Bit Server VM (build 25.112-b15, mixed mode)

git version 2.11.0.windows.3

node: v6.9.5

npm: 4.1.1

bower: 1.8.0

gulp: [13:09:40] CLI version 3.9.1

yeoman: 1.8.5

yarn: 0.19.1

Docker version 1.13.0, build 49bf474

docker-compose version 1.10.0, build 4bd6f1a0

jdubois commented 7 years ago

Can you put you JDL in the ticket ? I'm with my phone and I can't see it.

nicoraynaud commented 7 years ago

There you go :

entity Category {
    label String required
}

entity Bank {
    label String required
}

entity Currency {
    label String required,
    code String required,
    symbol String required,
    decimals Integer required
}

enum LineSource {
    MANUAL, GENERATED
}

enum LineStatus {
    NEW, TICKED, CANCELLED
}

enum BankAccountType {
    CHECKING, SAVINGS, OTHER
}

entity BankAccount {
    label String required,
    number String required,
    type BankAccountType required,
}

entity Line {
    date LocalDate required,
    label String required,
    debit Double,
    credit Double,
    balance Double,
    status LineStatus required,
    source LineSource
}

relationship ManyToMany {
    Line{categories} to Category
}

relationship OneToMany {
    BankAccount{lines} to Line{bankAccount}
}

relationship ManyToOne {
    BankAccount{bank} to Bank{bankAccounts},
    BankAccount{currency} to Currency
}

// Set DTO options
dto * with mapstruct except Category, Bank, Currency

// Set pagination options
paginate all with pagination

// Set service options to all except few
service all with serviceImpl except Category, Bank, Currency
jdubois commented 7 years ago

Thanks -> you whole object graph should be DTOs. Mixing DTOs and entities cannot work.

jdubois commented 7 years ago

Lazy-loading on a DTO cannot work, as it's not a managed object anymore.

nicoraynaud commented 7 years ago

My choice was motivated by the fact that referential data can remain entities while business data need dto. Anyway, I understand the motivation behind your answer.

However, letting the user choose which entity to expose as DTO and which not will lead to other cases like mine. Should we keep the choice ?

Thanks for your quick response !

jdubois commented 7 years ago

Yes you are right, it should be better documented, I'm re-opening this to remember doing it tomorrow

deepu105 commented 7 years ago

May be we could move the DTO question into the additional technologies to use question so that if its enabled its applied globally what do you think Julien?

Thanks & regards, Deepu

On 16 Feb 2017 10:10 p.m., "Julien Dubois" notifications@github.com wrote:

Yes you are right, it should be better documented, I'm re-opening this to remember doing it tomorrow

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5227#issuecomment-280461710, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF3dKoQ8vuS5WvAmYeJ7XJ8ZTl7svks5rdLtigaJpZM4MCgaf .

jdubois commented 7 years ago

No, it really depends on each entity. Sometimes you want entities, sometimes you want DTOs, it depends on what you are building. Then, if you select one option, you must be consistent: if you do graph of objects, all objects should be with the same technology, or it will just fail. For me this is quite logical, but indeed that's not documented correctly, and will be surprising for some people.

deepu105 commented 7 years ago

You are right in that case we could be smart about it and select dto automatically if any of the entity relationship has dto enabled. Its easy to do as we already read other entity json data

Thanks & regards, Deepu

On 20 Feb 2017 11:36 a.m., "Julien Dubois" notifications@github.com wrote:

No, it really depends on each entity. Sometimes you want entities, sometimes you want DTOs, it depends on what you are building. Then, if you select one option, you must be consistent: if you do graph of objects, all objects should be with the same technology, or it will just fail. For me this is quite logical, but indeed that's not documented correctly, and will be surprising for some people.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5227#issuecomment-281044311, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF8kc-P0g78gvvbkaO1G2KmomPn5Dks5reWy-gaJpZM4MCgaf .

jdubois commented 7 years ago

Oh yes, we could select/unselect DTO depending on the other entities which are linked... That would prevent such a issue.

jdubois commented 7 years ago

Reading the issue again, here you specifically ask:

dto * with mapstruct except Category, Bank, Currency

So it would be very strange to generate a DTO, when you explicitly ask not to have one! So that's not a good solution either...

deepu105 commented 7 years ago

Then we could atleast print a warning saying there is a misconfiguration

Thanks & regards, Deepu

On 21 Feb 2017 2:06 p.m., "Julien Dubois" notifications@github.com wrote:

Reading the issue again, here you specifically ask:

dto * with mapstruct except Category, Bank, Currency

So it would be very strange to generate a DTO, when you explicitly ask not to have one! So that's not a good solution either...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5227#issuecomment-281339304, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlFym9qegi5qK70ZoEib0Rv3R7lkgKks5reuFegaJpZM4MCgaf .

jdubois commented 7 years ago

Yes that's the easiest solution

jdubois commented 7 years ago

So I've tested this and:

So the end result is indeed, just to add a warning. The problem is that we currently don't "open" the other entities JSON configurations, so we can't know if they use mapstruct or not - and as a result it's quite hard to print that warning.

jdubois commented 7 years ago

Oh, and we ask the DTO question after doing the relationships, so that's too late to do the warnings... So I'm sorry but at the moment I can't find a solution.

cbornet commented 7 years ago

Could this be a warning/error in import-jdl task ?

deepu105 commented 7 years ago

@jdubois we do read the other entity json if present. Look at this so we might be able to write a warning if DTO is not selected

jdubois commented 7 years ago

OK, my bad! Can you re-open the ticket?

Le 7 mars 2017 6:58 PM, "Deepu K Sasidharan" notifications@github.com a écrit :

@jdubois https://github.com/jdubois we do read the other entity json if present. Look at this https://github.com/jhipster/generator-jhipster/blob/master/generators/entity/index.js#L565 so we might be able to write a warning if DTO is not selected

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5227#issuecomment-284803762, or mute the thread https://github.com/notifications/unsubscribe-auth/AATVo-EQQfD6Cuu5ltSfG60Vt__dAIYMks5rjZrAgaJpZM4MCgaf .