Closed Falydoor closed 6 years ago
+1 Not sure of what we can do here, but we shouldn't have a configuration that fails out of the box
The lazy initialization is a right approach. For such problem, a service layer would do its duty, fetching all data needed for the client which can have more than one DB calls. You don't need to fetch children entities only if you in the Hibernate session. A Hibernate session won't be kept alive in the modern web application architecture after fetching a parent entity.
If you change the line: "service all with serviceImpl except Employee, Job" by: "service all with serviceImpl"
If "Job" has service, it works correctly
the question here is:
EAGER
fetch type? I think that the original issue is that we try to serialize a "many" collection which is both an issue with lazy loading and for performance if the collection gets important. For instance on OneToMany relationship we hide the collection from the response.
Shouldn't we output a link to the collection instead in a very RESTful way ? This way, when the client calls the link we can apply proper pagination. And it could be applied to OneToMany relationships also.
The link could be something like /jobs/{id}/tasks
any result?
Please @Codefans-fan don't spam everyone if you don't work on the issue. If anything had been done, it would be written here.
The problem remains intact when using a service.
The generated Service classes are @Transactional
so the request the get the tasks is performed in the
for ( Task task : set ) {
(loop in JobMapperImpl).
Which actually leads to an N+1 selects problem the get all the jobs with all its tasks (that can be "solved" by a @BatchSize
like in the User class for the authorities).
And with no service it fails because the transaction is done.
A solution could be to have in EntityMapper
class:
public D toDtoLazy(E entity);
(with an @Mapping(ignore=true)
for the lazy collection)public List <D> toDtoLazy(List<E> entityList);
(and apply a mapstruct Qualifier to pick up the above mapping method instead of the 'default' one)There is a discussion about this: https://github.com/mapstruct/mapstruct/issues/778
JobMapper can be like this:
@Mapper(componentModel = "spring", uses = {TaskMapper.class, })
public interface JobMapper extends EntityMapper <JobDTO, Job> {
@Named("lazy")
@Mapping(target = "tasks", ignore = true)
JobDTO toDtoLazy(Job entity);
@IterableMapping(qualifiedByName = "lazy")
List<JobDTO> toDtoLazy(List<Job> entityList);
default Job fromId(Long id) {
if (id == null) {
return null;
}
Job job = new Job();
job.setId(id);
return job;
}
}
and in JobResource.getAllJobs method:
return new ResponseEntity<>(jobMapper.toDtoLazy(page.getContent()), headers, HttpStatus.OK);
@ctamisier I don't think we can address all use cases, your approach with toDtoLazy works for one relationship, what if you have several ones and some of them are bi-directional or creating a cycle?
Also, if you want to have efficient queries you should probably use @NamedEntityGraph
, so more code in entities and repositories.
I think this is the case where you must write ad-hoc mapper code and MapStruct just gets in your way.
I like @cbornet RESTful approach, it would produce a very intuitive swagger doc.
@deepu105 and @sendilkumarn this is the kind of tips that would be very useful in a JHipster book ;)
Other advantages of the REST link:
For the form, it could be done as an HATEOAS-compliant links
property in the response:
GET /api/jobs (paginated)
[ {
"id": 1001,
"jobTitle": "some job",
"links": [ {
"rel": "tasks",
"href": "http://localhost:8080/api/jobs/1001/tasks"
} ]
},
{
"id": 1003,
"jobTitle": "some other job",
"links": [ {
"rel": "tasks",
"href": "http://localhost:8080/api/jobs/1003/tasks"
} ]
}
]
GET /api/tasks (not paginated)
[ {
"id": 1002,
"title": "some task",
"links": [ {
"rel": "jobs",
"href": "http://localhost:8080/api/tasks/1002/jobs"
} ]
} ]
GET /api/tasks/1002/jobs (paginated) where task 1002 is linked to job 1001 but not to job 1003
[ {
"id": 1001,
"jobTitle": "some job",
"links": [ {
"rel": "tasks",
"href": "http://localhost:8080/api/jobs/1001/tasks"
} ]
} ]
yes very good. But we will still need a WS to have all the jobs with all its tasks in only one GET http call. (like we currently have with the list of users with all theirs authorities in one page)
@ctamisier if you mean eager fetching for performance reason because the client is sure to need the info, then this shouldn't be the default and I'm not sure we should generate something for that.
@jhipster/developers if we agree on the links, I can do the work for the backend but I would probably need help for the front... For instance, on the grid, the "many" value should probably be a link to the child grid filtered on the parent instead of the list of child IDs as we do currently.
For the backend, you don't need to do a lot of thing, after #5540 , the entity listing will be improved with filtering, so you can call : /api/tasks?jobId.equals=1001 and it will return the matching "tasks". However, from the performance point of view, adding an extra REST call is bad, the latency will kill the user experience.
@gzsombor that's the point of outputting a link : no automatic fetching of the child collections, only one HTTP query, only one Hibernate call.
But the browser still needs to issue a separate HTTP query to fetch the entities, isn't it ?
It depends on what you display in the browser 😃 For instance I don't think our current way of displaying all the child IDs is good if there is a lot of children (imagine if there are millions of them...). That's why I suggest to replace it by a link to a grid of the children filtered for the parent with proper pagination.
@gzsombor will the criteria filtering work with many-to-many relationships ?
I just realized that many-to-many are currently fetched eagerly in the repository (findAllWithEagerRelationships
, findOneWithEagerRelationships
). I don't think this is a good idea:
findAllWithEagerRelationships
is available but not used by default
findOneWithEagerRelationships
is used by default in the *Service.findOne
LAZY should always be the default
Yes, LAZY should be by default (it is currently the case) but the use findAllWithEagerRelationships and findOneWithEagerRelationships is required depending the cases:
If we have: Job <-*---*-> Task
:
findOneWithEagerRelationships
is justified because it is exactly what we want: the job with its tasks because we need to 'view' or 'edit' them in the according page.findAllWithEagerRelationships
could be justified if we need all the jobs with all the tasks in the jobs list page.If we have: Job <-*---*-> Task
and Job <-*---*-> Employee
:
findOneWithEagerRelationships
and findAllWithEagerRelationships
is not correct for me because the result of the SQL request will have NM lines (number of job-task relations number of job-employee relations).Eagerly fetching one level will not save you from LazyInitializationException if the graph is more than one level (A has many B, B has many C, ...)
True when we don't use DTO otherwise the methods in *Service class are @Transactionnal
and the mapping entity to DTO is done here (which causes eventual extra SQL requests) and the database can be hit at any time during this transaction (but can be suboptimal and having N+1 selects)
All this to say that I would keep findAllWithEagerRelationships
and findOneWithEagerRelationships
(and eventually make a different request when we have more than one @ManyToMany
relationships)
N+1 queries are not necessary bad when your related entities can be easily cached and don't change often. Just to say that we cannot cover all use cases and that our users are developers so they must understand the pros and cons in their business context.
Yes you're right.
In this case for this current issue should we just add @Transactional
to the Rest Controllers that don't have a service to have this working out of the box ?
@cbornet : in the current patch, the many-to-many relations are not covered yet (only the one-to-one and many-to-one), but it's fairly simple to add them.
@ctamisierfindAllWithEagerRelationships
is used. See https://github.com/jhipster/generator-jhipster/blob/master/generators/entity/templates/server/src/main/java/package/common/get_all_template.ejs#L30.
Again, what I'm saying is not that eager fetching is not sometimes the best solution but that it should be avoided if possible. As such JHipster generated code should avoid it and probably not generate it.
Here is the query used for TestManyToMany:
@Query("select distinct test_many_to_many from TestManyToMany test_many_to_many left join fetch test_many_to_many.testEntities left join fetch test_many_to_many.testMapstructs left join fetch test_many_to_many.testServiceClasses left join fetch test_many_to_many.testServiceImpls left join fetch test_many_to_many.testInfiniteScrolls left join fetch test_many_to_many.testPagers left join fetch test_many_to_many.testPaginations left join fetch test_many_to_many.testCustomTableNames")
List<TestManyToMany> findAllWithEagerRelationships();
I'm pretty sure that's inefficient. I'd rather have the user fetch the child collection he wants with another query. Or write his query by himself depending on his business context as @gmarziou said. So for me, default = lazy. If the user wants eager for his own valuable reasons, he writes the code himself. As for the JHipster generated code, I'd adapt the "List all" view to use only one lazy request and present links to view the child collections.
findAllWithEagerRelationships
is not used in the case of an entity generated with pagination. But the method is still here and can be used depending of the business context which I totally agree.
So ok I see, there will be a default behavior which is based on lazy fetching and then we let the developer optimize the requests if necessary.
@jdubois @cbornet @gzsombor @gmarziou @ctamisier sorry I didnt read the entire thread its very long so sorry if this was already discussed. Is the issue happening only when there is no service layer? we also had quite a number of issues in the past for such a combination. Would it make sense to enable service layer if DTO is chosen? I.e make service layer mandatory for DTO option. It might solve some head aches. I dont see why DTO needs to be used without a service layer as they are already in the service layer and we even recommend using service with DTO. WDYT?
The only configuration that does not make much sense to me is DTO without service layer, then there can still be use cases for this. So I would keep the options and either solve the issue or at least warn the user.
Le 6 août 2017 11:58 AM, "Deepu K Sasidharan" notifications@github.com a écrit :
@jdubois https://github.com/jdubois @cbornet https://github.com/cbornet @gzsombor https://github.com/gzsombor @gmarziou https://github.com/gmarziou @ctamisier https://github.com/ctamisier sorry I didnt read the entire thread its very long so sorry if this was already discussed. Is the issue happening only when there is no service layer? we laos had quite a number of issues in the past for such a combination. Would it make sense to enable service layer if DTO is choosen? I.e make service layer mandatory for DTO option. It might solve some head aches. I dont see why DTO needs to be used without a service layer as they are already in the service layer and we even recommend using service with DTO. WDYT?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5629#issuecomment-320497098, or mute the thread https://github.com/notifications/unsubscribe-auth/AATVo5x0OWirRwPtl1TOrMEPQ1a-0XaBks5sVY5MgaJpZM4NChK8 .
My position is still the same :
The main problem for that approach would be generation since we don't know which entities point to the entity we are generating.(we know if a one-to-many is unidir when generating the many side since it's the owner of the relationship in this case, and not when the rel is bidir)
I started to look at what would need to be generated to have child collections fetch endpoints. @gzsombor 's filter endpoints would be nice to use but as of now they are optional and don't support many-to-many. So writing new resource code, this gives for bidir OneToMany JDL
entity Car {
name String
}
entity Person {
name String
}
relationship OneToMany {
Person{ownedCar} to Car{owner},
Person{drivenCar} to Car{driver}
}
CarResource.java
/**
* GET /people/:id/owned-cars : get the owned-cars for the owner "id".
*
* @param id the id of the person for which to get the owned-cars
* @return the list of owned-cars for the person "id"
*/
@GetMapping("/people/{id}/owned-cars")
@Timed
public ResponseEntity<List<CarDTO>> getCarsByOwnerId(@PathVariable Long id, @ApiParam Pageable pageable) {
log.debug("REST request to get owned-cars for person : {}", id);
Person person = new Person();
person.setId(id);
Page<Car> page = carRepository.findByOwner(person, pageable);
HttpHeaders headers = PaginationUtil.generatePaginationHttpHeaders(page, "/api/people/" + id + "/owned-cars");
return new ResponseEntity<>(carMapper.toDto(page.getContent()), headers, HttpStatus.OK);
}
/**
* GET /people/:id/driven-cars : get the driven-cars for the driver "id".
*
* @param id the id of the person for which to get the driven-cars
* @return the list of driven-cars for the person "id"
*/
@GetMapping("/people/{id}/driven-cars")
@Timed
public ResponseEntity<List<CarDTO>> getCarsByDriverId(@PathVariable Long id, @ApiParam Pageable pageable) {
log.debug("REST request to get driven-cars for person : {}", id);
Person person = new Person();
person.setId(id);
Page<Car> page = carRepository.findByDriver(person, pageable);
HttpHeaders headers = PaginationUtil.generatePaginationHttpHeaders(page, "/api/people/" + id + "/cars");
return new ResponseEntity<>(carMapper.toDto(page.getContent()), headers, HttpStatus.OK);
}
As for hypermedia links if we want to add them for documentation/ease-of-use, it's easy to add them using spring-hateoas. PersonResource.java
@GetMapping("/people/{id}")
@Timed
public ResponseEntity<PersonDTO> getPerson(@PathVariable Long id) {
log.debug("REST request to get Person : {}", id);
Person person = personRepository.findOne(id);
PersonDTO personDTO = personMapper.toDto(person);
if (personDTO != null) {
personDTO.getLinks().add(linkTo((methodOn(CarResource.class).getCarsByDriverId(person.getId(), null))).withRel("drivenCars"));
personDTO.getLinks().add(linkTo((methodOn(CarResource.class).getCarsByOwnerId(person.getId(), null))).withRel("ownedCars"));
}
return ResponseUtil.wrapOrNotFound(Optional.ofNullable(personDTO));
}
which will output for GET http://localhost:8080/api/people/1
{
"id": 1,
"name": "qrehgtrh",
"links": [
{
"rel": "drivenCars",
"href": "http://localhost:8080/api/people/1/driven-cars"
},
{
"rel": "ownedCars",
"href": "http://localhost:8080/api/people/1/owned-cars"
}
]
}
Sorry, I've started implementing the many-to-many stuff, the library part is done, here. After it's merged, and a new release is made, then the generator can be adjusted.
For the two-way references between a many-to-many relation (or with any other type of relation), is not solved automatically. Maybe we could re-generate both the entities in this case?
Other than, I don't get why this hateoas thing is good. Yes, with generating that many links, you could have a very generic client, which could automatically download a lots of entities, without knowing the meaning of them. Which is nice, if you want to crawl your site :-) But if you develop an actual client, then you will know, that people entity have 'drivenCars' and 'ownedCars' property, and in this case, following REST rules, you could be expecting them reachable under {entity URL}/driverCars and {entity URL}/ownedCars, instead of this additional indirection. If not, I would consult the generated swagger documentation :)
@gzsombor the intent with the links is not to provide HATEOAS / REST level 3. It's just to provide nice-to-have auto-documentation for the humans, not expecting smart clients to use them at runtime.
Hi, Not sure this is here (relates more or less to same kind of issue) I should ask but: With a new project generated I have relationships of ManyToOne and default generated code does not define it as LAZY (even for OneToOne), as a result for DTOs mapper that were generated as well it expects those relationship as being fetched. Problem it results in N+1 queries for the app for default entities management pages generated by jhipster. Of course if I change all relationship to LAZY then I get exception as session was closed, so it would require to write manually join fetch in repositories to make that work (takes time, especially when have more than 150 entities).
Is it normal that jHipster does not define all relationship as LAZY? If could generate as LAZY then why not define in repositories a EntityGraph that fetches this xToOne relationship in order to show these entity management page?
Was this choice to reflect JPA definition of relationships? But Hibernate (and even high-performance-java-persistence book) recommend to use Lazy everywhere then do join fetch per query basis.
Thank you.
@Blackdread please don't pollute the thread, for general questions please use stackoverflow (and I don't think JHipster does eager relationships by default anywhere)
Sorry for my previous comment @Blackdread - I see that in fact you are an Hibernate master (in ticket #6105 ) and that I should listen to you more :-)
Anyway: this as been opened for too long, and I'm not sure we can find an easy solution. I just don't have enough time at the moment to work on this - @gzsombor @cbornet you are probably the 2 best experts here, what do you think? Should we close this?
There is really a bug so I don't think we can close this without any fix...
It's been a long time. I don't know whether this bug is fixed or not.
A solution that worked for me is:
@ManyToMany(fetch = FetchType.EAGER)
@BatchSize(size=100)
Put it in the entity that have ManytoMany relationship.
It will reduce considerably the number of query in case the children entities is too large.
Hope this help.
@apovn no it has not been fixed, the bug is still opened. But nobody works on this... As written several times in this thread, in particular by @cbornet , doing an Eager fetch is usually a code smell. Maybe it works in your use case, but I agree with @cbornet we shouldn't generate this by default anywhere in our code (which doesn't mean you shouldn't change our default configuration, depending on your business needs)
I don't know why nobody works on this because it throws an exception if I don't add the code as I mentioned in previous comment.
In my code: Customer and Ticket have ManyToMany relationship. When I list all Tickets (/api/tickets), it throws Exception below: Line 74:
Do you have a solution for this problem, @jdubois ?
Nobody is working on it because that doesn't seem to interest anyone... People just have better things to do, that's all. I would have a solution if I had time to work on this: as far as I'm concerned, MapStruct is still marked BETA, and I'm not using it personally, so this is not my priority.
ok, i understand. thank you.
The better solution is to fix the transaction handling :
Basically, you need to access your database from a code block, which is in a 'JPA/hibernate session'
@gzsombor @jdubois what you think of showing DTO and Filter option only when Service is selected? it will atleast solve these kind of issues
Yes, that would make sense !
I like it that we offer the options to have "no DTOs and a service layer", and "no service layer and DTOs". But indeed, it doesn't make much sense to have DTOs without a service layer, and this seems to constantly cause trouble to people. We should at the very least issue a warning in the question. People should understand why they choose an option, so if you have DTOs but no service layer, well you should be aware you will get lazy initialization errors. And if you select this, then you're big enough to handle with it. If that's not enough maybe remove the option, OK - but that means this would have impact on the JDL, so maybe that's too much work.
@jdubois what I can do is issue the warning if the options are "incompatible" at JDL-parsing time.
Yes I would also prefer to suppress the DTO and filtering option when service is not selected. It will reduce lot of headaches for us
Thanks & Regards, Deepu
On Mon, Dec 4, 2017 at 1:59 PM, Mathieu ABOU-AICHI <notifications@github.com
wrote:
@jdubois https://github.com/jdubois what I can do is issue the warning if the options are "incompatible" as JDL-parsing time.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/5629#issuecomment-348955424, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF1-MheiTIQwNp5PXoBoT5aXUxp2nks5s8-zNgaJpZM4NChK8 .
I have disabled DTO if service is not selected this won't affect existing apps and JDL as currently it only disables the prompt. @MathieuAA it would be nice if we can do the warning from JDL as well
Overview of the issue
Hibernate throws an exception when trying to get all Job entities after creating a Job entity.
Motivation for or Use Case
This issue only appears when having an entity with a ManyToMany relationship, being paginated (pager, pagination or infinite-scroll) and dto generated with mapstruct. It's a little specific but if you use the default JDL from JDL Studio you will have the issue.
Reproduce the error
Use the JDL below to generate entities. Create a Job entity with no Task. An exception will be thrown when displaying all Job.
Related issues
Found nothing related.
Suggest a Fix
Maybe by doing an eager load on the ManyToMany relationship like it's done when Job is not paginated.
JHipster Version(s)
4.3.0
JHipster configuration
JHipster Version(s)
JHipster configuration, a
.yo-rc.json
file generated in the root folderEntity configuration(s)
entityName.json
files generated in the.jhipster
directoryJob.json
Task.json
Browsers and Operating System
java version "1.8.0_92" Java(TM) SE Runtime Environment (build 1.8.0_92-b14) Java HotSpot(TM) 64-Bit Server VM (build 25.92-b14, mixed mode)
git version 2.10.1 (Apple Git-78)
node: v7.8.0
npm: 4.2.0
bower: 1.8.0
gulp: [23:47:04] CLI version 1.2.2 [23:47:04] Local version 3.9.1
yeoman: 1.8.5
yarn: 0.22.0
Docker version 17.03.1-ce, build c6d412e
docker-compose version 1.11.2, build dfed245
Entity configuration(s)
entityName.json
files generated in the.jhipster
directoryJDL
Browsers and Operating System
Chrome 57 on OS X El Capitan