Generator-jhipster: Service layer depends on web layer

[kaidohallik]

Overview of the issue

In https://github.com/jhipster/generator-jhipster/issues/4867 was discussed that calling web layer from service layer is an architecture error. And this was fixed by https://github.com/jhipster/generator-jhipster/pull/4877

But from https://github.com/jhipster/generator-jhipster/pull/7983 service layer depends again on web layer because of https://github.com/jhipster/generator-jhipster/commit/cc547cabb35566af8467318d88509e082db100f2#diff-eb8ab57e6c1b5729372ec3081226b95dR43

Current version of this code: https://github.com/jhipster/generator-jhipster/blob/master/generators/server/templates/src/main/java/package/service/UserService.java.ejs#L45

Motivation for or Use Case

Reproduce the error

Related issues

Similar topic is discussed in this SO question: https://stackoverflow.com/questions/40116314/jhipster-exception-handling-in-services

Suggest a Fix

Possible fix:

• create package error.rest here: https://github.com/jhipster/generator-jhipster/tree/master/generators/server/templates/src/main/java/package
• move the following classes from https://github.com/jhipster/generator-jhipster/tree/master/generators/server/templates/src/main/java/package/web/rest/errors to this new location:
  
  
  
  • BadRequestAlertException.java.ejs
  
  
  • EmailAlreadyUsedException.java.ejs
  
  
  • EmailNotFoundException.java.ejs
  
  
  • ErrorConstants.java.ejs
  
  
  • InvalidPasswordException.java.ejs
  
  
  • LoginAlreadyUsedException.java.ejs
  
  

JHipster Version(s)

[email protected] C:\projects\teest\jhipster-web-rest-errors
`-- (empty)

##### **JHipster configuration, a `.yo-rc.json` file generated in the root folder**

.yo-rc.json file

{
  "generator-jhipster": {
    "promptValues": {
      "packageName": "com.mycompany.myapp",
      "nativeLanguage": "et"
    },
    "jhipsterVersion": "6.0.1",
    "applicationType": "monolith",
    "baseName": "jhipster",
    "packageName": "com.mycompany.myapp",
    "packageFolder": "com/mycompany/myapp",
    "serverPort": "8080",
    "authenticationType": "jwt",
    "cacheProvider": "ehcache",
    "enableHibernateCache": true,
    "websocket": false,
    "databaseType": "sql",
    "devDatabaseType": "h2Disk",
    "prodDatabaseType": "mysql",
    "searchEngine": false,
    "messageBroker": false,
    "serviceDiscoveryType": false,
    "buildTool": "gradle",
    "enableSwaggerCodegen": false,
    "jwtSecretKey": "bXktc2VjcmV0LXRva2VuLXRvLWNoYW5nZS1pbi1wcm9kdWN0aW9uLWFuZC10by1rZWVwLWluLWEtc2VjdXJlLXBsYWNl",
    "useSass": true,
    "clientPackageManager": "npm",
    "clientFramework": "angularX",
    "clientTheme": "none",
    "clientThemeVariant": "",
    "testFrameworks": [],
    "jhiPrefix": "jhi",
    "entitySuffix": "",
    "dtoSuffix": "DTO",
    "otherModules": [],
    "enableTranslation": true,
    "nativeLanguage": "et",
    "languages": [
      "et"
    ]
  }
}

JDL for the Entity configuration(s) entityName.json files generated in the .jhipster directory

JDL entity definitions

Environment and Tools

java version "1.8.0_211"
Java(TM) SE Runtime Environment (build 1.8.0_211-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.211-b12, mixed mode)

git version 2.21.0.windows.1

node: v10.15.3

npm: 6.9.0

yeoman: 2.0.6

yarn: 1.15.2

Docker version 18.09.2, build 6247962

docker-compose version 1.23.2, build 1110ad01

Browsers and Operating System

Windows 10

• [x] Checking this box is mandatory (this is just to show you read everything)

[pascalgrimaud]

Thanks @kaidohallik
You're right, it is important !

[pascalgrimaud]

@cbornet : what do you think about the suggested fix plz ?

[jdubois]

Very good catch @kaidohallik !! That's indeed a very important issue (I've adding a bug bounty here for this reason).

Now we are in trouble here :

• As this exception inherits from "Zalando problem", this is linked to Web APIs (it even has a URI in it), so for me it should stay in the Web layer.
• But this logic should be in the service layer.
• Also, this method shouldn't be split: it's good to have a transaction around everything here.

So we're blocked. And we can't do a big refactoring here until our next major version, as this would break to many things.

So 2 solutions for me:

• We refactor the current method in 3, so the exception is thrown from the Web layer -> not beautiful, bad for the transaction, but easy and no other consequences
• Move the exceptions to the "service" layer -> we'll need to wait our next major version

@cbornet @pascalgrimaud WDYT?

[atomfrede]

What about having something like runtime execption which is thrown from within the service layer and have an exception handler that handles and translates these exceptions (if not catched by the weblayer) into corresponding webexceptions/problems?

[jdubois]

@atomfrede that would be the cleanest solution, but I'm reluctant to add our own exception system here.

[jdubois]

Or maybe we could move the exception to a JHipster framework package. People would then need to extend them if they want to modify them, but that would make upgrading easier for them.

[atomfrede]

@atomfrede that would be the cleanest solution, but I'm reluctant to add our own exception system here.

As we already have our own exceptions I think having a dedicated exception handler advice which handles our own service layer exceptions, maps them to a problem and let the problem exception handler to the rest (like today) is not much code, but I agree maybe it is something we should put into our server lib.

[cbornet]

Yes it's probably better to have service errors independent from the way they will be represented and controller advices to translate it in the web layer.

[cbornet]

Or maybe they should be catched and rethrown in the web layer because you don't necessarily know what should be the http status after a service layer exception.

[Blackdread]

For me it is normal to throw custom exception in service layer, it is usually always translated in web layer via ControllerAdvice, and any "Exception" not already translated is modified to a generic web layer (which is done already in jHipster).
You may still catch the exception in controller if default translation does not suit your needs for that particular controller (even an exception handler method in the controller to reduce the scope instead of the try/catch)
https://spring.io/blog/2013/11/01/exception-handling-in-spring-mvc

[atomfrede]

Should be build and example app? We could also add arch-unit to test those "simple" architecture rules, such that don't happen again (or at least the generated app will have failures). What do you think?

[jdubois]

Yes we should first have a very simple proof of concept so that everyone can discuss. This could be quite a significant change, I'm not sure everybody will agree.

[atomfrede]

I will try to build an example during the week so we can discuss it a bit further.

[atomfrede]

I have setup a small repository with a first proposal, open for discussion. https://github.com/atomfrede/jh-9876/commits/master

• I have added ArchUnit and a simple test to check services and repositories do not depend on the web layer
• Renamed the exception in the web layer (maybe we could use Problem instead of WebException)
• Added new exceptions as runtime exceptions
• special handler methods for these exceptions translating them to there corresponding web/problem counterparts

[atomfrede]

@jhipster/developers Anyone had time already to look into the changes from :arrow_up:

[Tcharl]

@atomfrede reread, seems legit to me

[atomfrede]

If everyone agrees with the solution and using arch unit to check such constraints I will try to create a first PR during the weekend

[pmverma]

+1 from me as it enforces users to keep the architecture cleaner and consistent.