Move the logic of converting criteria to specification out of QueryService.

[yelhouti]

Overview of the feature request

In the current generator, it is the Job of the EntityQueryService to convert the criteria into a specification, the problem with this approach is that it prevents from easily filtering relationships based on these fields if the entity is not marked as filterable. Also all the logic of the of createSpecification is static and should not require retrieving a service and allocating resources for it. IMO, teck.jhipster.queryService that has all buildSpecification methods be deprecated in favor of a helper class called SpecificationHelper with all public static methods.

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

[gzsombor]

This is not accidental, this is the intended design, to allow easy customization. For example, if you want to add a 'like' filter to any Filter class, you could create a custom subclass for StringFilter, and in your service classes, you can override/extend the appropriate buildSpecification(...) with your new logic, and it's done, you can use it everywhere. If this were a mix of static functions calling each other, you couldn't do it. And I don't think,that creating one service class per entity type is a measurable difference in memory consumption and performance - without having lot of field, it's at most 30-40 bytes/instance.

[yelhouti]

@gzsombor you ignored my main complaint : avoid creating the whole QueryService of an entity just to be able create a specification from a criteria. And although it is the intended design, this was designed when we had flattened DTOs, and it doesn't mean it is a good design (anymore).

[gzsombor]

But why do you want to avoid creating a service? Creating services/components is the way of programming in Java - at least since the rise of Spring. We can argue about, how big a service should be, and how to separate things, here, the EntityQueryService is responsible to converting a criteria into a list of matching entities, with all the small, nearly one liner functions hidden in the ancestor type. Sorry, I don't understand the flattened DTO argument, why is it relevant, what the output structure is?

[yelhouti]

We can create a query service for each entity, even if it is not marked filterable in the JDL, but in that case I would argue that we should move some of the logic around. from an aesthetics perspective, I don't like having a EntityQueryService with the findByCriteria that is never used. I would rather move the findByCriteria to the EntityService when needed (entity filterable, pageable...) and have the createSpecification be the only method in the EntityQueryService (that we can rename) and inject it the main service.

I would really like other people weighing in on this, to discuss how it should be done.

Many opinions are better than two ;) .

[gmarziou]

I'm in favor of keeping current service because it's easier to mock for testing than a static helper.

[yelhouti]

@gmarziou good point, how about the other changes? generating the QueryService for all entities (maybe renaming it) with only createSpecification and move [findBy|count]ByCriteira to the EntityService when the entity if filterable/pageable... ?

[gmarziou]

@yelhouti maybe it would help if you could provide a sample project with a commit showing your proposed changes.

[gzsombor]

So you want to generate the criteria object, and part of the EntityQueryService - which deals with the criteria -> specification conversion, no matter if the user asked for filtering or not, and move the actual 'specification' usage to somewhere else, don't you? I think in this case, the following two solution would make more sense:

• generate the whole criteria and EntityQueryService by default, without having a way to disable it by the user.
• generate the whole criteria and part of the EntityQueryService which deals with the conversion, and generate the find and count methods only when filtering is turned on.

As I can't see reason to actually generate CRUD without filtering, I always turn it on, and the only reason I can think of to not do it, is to actually reduce the amount of generated code with that 2 classes. But now, you want to have most of it generated. Sooo...

[yelhouti]

@gmarziou Isure let me get back to you with that.

@gzsombor If we decide to always generate that code (which I don't mind, but I understand why others might), I would still argue that code should be moved around (I understand that the other ways you suggested are even easier to code and get approved), but,

• I believe similar stuff should be grouped (findAll and findAllByCriteria should be in the same Service if they are always present)
• The QueryService is now lazily injected in other QueryServices and circular dependencies should be reduced to a minimum to avoid runtime problems.

As I can't see reason to actually generate CRUD without filtering, I always turn it on, and the only reason I can think of to not do it, is to actually reduce the amount of generated code with that 2 classes. But now, you want to have most of it generated. Sooo...

That is the thing, if we are always generating it, reorganizing is paramount to avoid confusing people. Splitting does not make sense anymore, unless we split differently (as suggested above for the reasons explained above).

[gzsombor]

Yeah, findAll and findAllByCriteria could go into the same service, valid argument. I usually end up with putting every querying and listing code into the EntityQueryService, and putting just the creating/updating/deleting logic into the EntityService - this works pretty well (And putting the business logic - which modifies the entities - into the individual EntityServices, and later move to separate services as they grow..) That way, you dont need to depend on the entity modification service everywhere, you can freely inject the query services every place, where you need one, or more entities.
Why do you want to create circular dependencies with QueryServices? That would be a dangerous minefield to walk on...

[yelhouti]

@gzsombor galdly we agree on grouping.

Why do you want to create circular dependencies with QueryServices?

To allow both sides of an entity to query based on the other.

That would be a dangerous minefield to walk on...

Not really if the only thing it does is convert criteria to specification,

[mshima]

Yeah, findAll and findAllByCriteria could go into the same service, valid argument.

I prefer to not unify then. I have plans to override EntityQueryService at jOOQ blueprint. An alternative is to make EntityQueryService extend EntityService.

[yelhouti]

@mshima

An alternative is to make EntityQueryService extend EntityService.

I don't understand, wouldn't that unify them.

I have plans to override EntityQueryService at jOOQ blueprint.

Your blueprint could still do that.

We could also just move the logic of creatSpecification from criteria to another service, that would be good enough for me (we can inject it into the queryService (and other queryServices)

[gzsombor]

@gzsombor galdly we agree on grouping.

Why do you want to create circular dependencies with QueryServices?

To allow both sides of an entity to query based on the other.

That would be a dangerous minefield to walk on...

Not really if the only thing it does is convert criteria to specification,

Sorry, I don't understand what you are trying to say. If there is no circular references in the code which converts a criteria to a specification, there shouldn't be a circular reference in the code, which gets the specification, ask JPA to execute it, and return the list of the found entities.

[yelhouti]

@gzsombor you are right I am just tiered, one of the main raisons of extracting the logic from the queryService is to avoid circular dependencies...

[gmarziou]

I usually end up with putting every querying and listing code into the EntityQueryService, and putting just the creating/updating/deleting logic into the EntityService

CQRS inspiration here, I like it :) They could even be named as EntityQueryService and EntityCommandService.

[pblanchardie]

An alternative is to make EntityQueryService extend EntityService

Please don't! ;)

I usually end up with putting every querying and listing code into the EntityQueryService, and putting just the creating/updating/deleting logic into the EntityService

CQRS inspiration here, I like it :) They could even be named as EntityQueryService and EntityCommandService.

Exactly, that's what I used to do, and it's definitely cleaner. Sometimes, the CommandService needs to inject the QueryService, but not the other way!

I have plans to override EntityQueryService at jOOQ blueprint.

Me too :)

In projects with complex querying, it's nice to be able to extend QueryService with custom logic and security rules, and keep things separated.

I'm not in favor of unifying reads and writes.

But moving Specifications to separate classes can make sense (they become beautiful), and can lead to cleaner customization and usage of Specifications.

public class EntityFilteringSpecification implements Specification<Entity>
    private EntityCriteria criteria;

Or

public class EntitySpecifications
    public static Specification<Entity> filtering(EntityCriteria criteria)

[yelhouti]

As long as we agee on what to do, I'm ok writing the code, core team should decide I guess, so what do you guys think

[github-actions[bot]]

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

[yelhouti]

can any one reopen this please?

[yelhouti]

@pascalgrimaud maybe since your moved it to the milestone.

[pascalgrimaud]

I put this ticket in the milestone automatically, for all closed tickets :-)

[yelhouti]

I know I know :D just teasing :p forgot ma emots :p

[yelhouti]

Here is an example of what we could Have: https://github.com/boutainaLemrabet/moveSpecificationToCriteriaDemo Feedback much appreciated

[deepu105]

@gzsombor is the boss here so I'll leave the final decision to him

I have few comments/requests though

1. Can we keep things as simple as possible, I don't think we need more fancy patterns and so on
2. Please do not generate criteria service/code by default. We already generate a lot of code and we don't want to do even more
3. Can we put our personal expectations of the perfect code aside and do what is best for our average user (this person might be using JHipster to make his/her life a bit more easier and not to spend more time understanding complexity we add)

[gzsombor]

Here is an example of what we could Have: https://github.com/boutainaLemrabet/moveSpecificationToCriteriaDemo Feedback much appreciated

I don't think this solution is good, I would rather not put complex and internal implementation related code into a class, which is used as a DTO (a criteria class is just a DTO, with some Spring magic, so it can be converted from the URL parameters automatically). And as these DTOs and Criteria objects don't depend on a heavy jar dependency chain (Only that ~10 Filter classes from jhipster-framework are needed), you can easily provide a separate client library for your service, with these classes.

Maybe, you could implement this Criteria -> Specification conversion with MapStruct, which would mean, that we don't add too much newly generated complex code to our users codebase (Because we would generate that during compile time :tada: ), but it's just a wild idea.

Generally my concern is still that, I can't see if the added complexity worth it. The goal of the entity filtering is to provide an easy way for the UI to list entities - 'get me all the orders which is in X state for customer 123' etc. But it was never a goal to provide a DSL or a query language to build arbitrary complex queries. If you need that, then use GraphQL, or if the users needs custom visual query building, then use Redash/Metabase/Baserow/Superset etc UI.

[yelhouti]

@gzsombor you are making very good points here so, let me explain what the problem was, and maybe we could find a better solution.

Our goal is to Unflatten the Criteria class. This is needed for filtering when you have composite Ids and want to filter by the other class field (I can give more detail on why if needed).

The thing is once we have Unflattened criteria they might be cyclic references, in a simple example where employees work at the same time on different Tasks. The TaskCriteria has a field EmployeeCriteria and vise versa.

Therefore if we have a TaskQueryService that converts a TaskCriteria to a Specification<Task> it will need to inject the EmployeeQueryService. (this cyclic dependency can be handled by @Lazy).

The problem occurs for example when the other entity doesn't have a QueryService we could still create a Criteria. convert it to a Specification. But we do not need to have the full QueryService that has findAll ... because it is not needed.

The fix: split the QueryService into two parts: 1- a service that converts to specification called CriteriaService or SpecificationService. 2- a QueryService that does the findAllByCriteria coundByCriteria PageByCriteria that injects the first one.

Another question arrises: When to generated The Criteria class, CriteriaService and QueryService: For the QueryService it's easy, the same as today, when the entity is filterable or pageable. For the Criteria and its Service well one solution would be to do the same but it would prevent filtering on relationships, this doesn't look good enough for me, the second solution is that if one of the related relationships is filterable generate the CriteriaService two.

please le me know what you think.

@deepu105 here is your ping :p.

Edit: for the CriteriaService MapStruct won't work because it doesn't handle @Lazy/cyclic AFAIK. we would need to generate them and inject them in each other using @Lazy ourselves

[deepu105]

Why are we talking about reactive and non reactive in the same app? Are we allowing one entity to be reactive and other to be normal (I haven't followed the reactive impl)

I'm again gonna repeat what said before. Don't try to get everything working. Its ok for some options to not work with another. You are really making things more complicated for a combination that is not the most common. I would have simple generated code for most of our users rather than providing a fancy combination for some of them

On Wed, 3 Mar 2021, 11:17 pm yelhouti, notifications@github.com wrote:

@gzsombor https://github.com/gzsombor you are making very good points here so, let me explain what the problem was, and maybe we could find a better solution.

Our goal is to Unflatten the Criteria class. This is needed for filtering when you have composite Ids and want to filter by the other class field (and can give more detail on why if needed).

The this is once we have Unflattened criteria they might be cyclic references, in a simple example where employees work a the same time om different Tasks. The TaskCriteria has a field EmployeeCriteria and vise versa.

Therefore if we have a TaskQueryService that converts a TaskCriteria to a Specification it will need to inject the EmployeeQueryService. (this cyclic dependency can be handled by @lazy https://github.com/lazy).

The problem occurs for example when the other entity doesn't have a QueryService (example Reactive User can't jave a Query Service because they use r2bc instead of JPA) we could still create a UserCriteria. convert it to a Specification. But we can not have a QueryService that has findAll ... because UserRepository doesn't allow for it.

The fix: split the QueryService into two parts: 1- a service that converts to specification called CriteriaService or SpecificationService. 2- a QueryService that does the findAllByCriteria coundByCriteria PageByCriteria that inject the first one.

Another question arrises: When to generated The criteria class, criteriaService and queryService: For the queryService it's easy, the same as today, when the entity is filterable or pageable. For the criteria and it's converter well one solution would be to do that same but it would prevent filtering on relationships which doesn't look good enough for me, the second solution is that if one of the reltedRelationships is filterable generate the two classes.

please le me know what you think.

@deepu105 https://github.com/deepu105 here is your ping :p.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/13133#issuecomment-790112486, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIOKF5UDLI2ZTPZJICYEE3TB2YP3ANCNFSM4UAMWDQA .

[yelhouti]

@deepu105

Why are we talking about reactive and non reactive in the same app

We are not. dumb example, you can ignore that part. I changed it.

we just keep in mind that filtering doesn't work with reactive.

[yelhouti]

@deepu105 any other remarks, feedback? should I go forward and implement this? should I wait for @gzsombor or give you more time to think about it ?

[deepu105]

As I said, @gzsombor has to sign off on this one as he is in charge of this area. I would still suggest you go ahead with a simplest path which involves less breaking changes and just disable options that wont work together

[yelhouti]

@deepu105 you mentioned disabling options that won't work well together, I totally get that. But are you talking about this Issue or the composite id PR?

The PR for this is quite straightforward so I went ahead and implemented it. https://github.com/jhipster/generator-jhipster/pull/14180 It also unflattens criteria the same way we did for DTO. This is a building block for composite, I tried to follow what you suggested here: https://github.com/jhipster/generator-jhipster/pull/13860#issuecomment-789539634

1.) Move all changes which has nothing much to do with composite keys but are generic improvements into the first PR so that we can merge it for v7

Hopefully @gzsombor approves it :)

[gzsombor]

So, my points: 1, I still don't understand, how this relates to composite keys, even in the future, how can be a building block for composite. IF the composite keys are just regular fields, they could work out of box with the current filtering. 2, I still don't understand, how this can be useful for more than 1 people - apart from the technical curiosity. I mean, if you want to have a generic querying capabilities, use GraphQL, and you will get a couple nice client libraries and UI with it. And you can create a GraphQL generator in a blueprint. 3, This solution still has cyclic references, with @Lazy bean initialization - which is a bit painful, if you ever want to have a simple tests with a couple of services linked together.... 4, So creating 2 classes instead of one, doesn't really solve anything, just add number-of-entities new classes. Generating EntityCriteriaService without EntityQueryService doesn't make too much sense, if you are still generating a lot of code, even when the user doesn't want filtering on that entity.

[yelhouti]

@gzsombor thank you for making your points clear, please let me try address those: 1- When using compositeKeys, we often don't want to filter by the id of the other field instead what would like to filter by it's name..., this becomes more and more complicated to code if the entity is far away you need to re-code the join logic in each entity. In this repo example: https://github.com/yelhouti/composite-expected For the entity EmployeeSkillCertificate, if you want to find all certificated for a specific user by it's firstname. you will need to code the join in EmployeeSkillCertificateQueryService and if you want to find skills by employee a subset if these joins needs to be coded again in EmployeeSkillQueryService. In the proposed code, transforming a a criteria to a specification is done speratly and JoinLogic is coded once, it can then be reused for other classes. 2- Why do we need this? in the Create/Update form we only want to show possible combinations. Ex: When creating an employeeSkillCertificate, we want to be able to choose between EmployeeSkill, clearly many o those exist, so we would first choose the skill first for example, then chose all EmployeeSkills for that specific skill, this kinf of filtering become mondatory.

3- indeed by the nature of what we re trying to achieve is cyclic, but if you have a cleaner approach ?am all ears.

4- We are only generating classes if we want one of it's the related entities to be filterable (and not all entities) this can be naround down very easily. add an option @criteria and based on if it's present generate those criteria and Service. Also spliting the logic is based on seperation of concerns principal as discussed with @gmarziou above.

Please don't hesitate if you have more concerns.

[deepu105]

Can we just drop this and disable composite keys when filtering is enabled and vice versa. I don't see any value in this added complexity TBH. All this is just reiterating my personal opinion that composite keys are a bad idea other than for very specialised use cases.

On Sat, 6 Mar 2021, 12:59 pm yelhouti, notifications@github.com wrote:

@gzsombor https://github.com/gzsombor thank you for making your points clear, please let me try address those: 1- When using compositeKeys, we often don't want to filter by the id of the other field instead what would like to filter by it's name..., this becomes more and more complicated to code if the entity is far away you need to re-code the join logic in each entity. In this repo example: https://github.com/yelhouti/composite-expected For the entity EmployeeSkillCertificate, if you want to find all certificated for a specific user by it's firstname. you will need to code the join in EmployeeSkillCertificateQueryService and if you want to find skills by employee a subset if these joins needs to be coded again in EmployeeSkillQueryService. In the proposed code, transforming a a criteria to a specification is done speratly and JoinLogic is coded once, it can then be reused for other classes. 2- Why do we need this? in the Create/Update form we only want to show possible combinations. Ex: When creating an employeeSkillCertificate, we want to be able to choose between EmployeeSkill, clearly many o those exist, so we would first choose the skill first for example, then chose all EmployeeSkills for that specific skill, this kinf of filtering become mondatory.

3- indeed by the nature of what we re trying to achieve is cyclic, but if you have a cleaner approach ?am all ears.

4- We are only generating classes if we want one of it's the related entities to be filterable (and not all entities) this can be naround down very easily. add an option @criteria https://github.com/criteria and based on if it's present generate those criteria and Service. Also spliting the logic is based on seperation of concerns principal as discussed with @gmarziou https://github.com/gmarziou above.

Please don't hesitate if you have more concerns.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/13133#issuecomment-791924566, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIOKFZCZVOR7ANZQQNOUNTTCIKLLANCNFSM4UAMWDQA .

[yelhouti]

@deepu105 I understand why people might not like composite keys, for me I find it very useful for validation...

That said, I used to have all my work in blueprints because you (main contributors) didn't like it. But since the blueprints have been used more than a few times, I have been asked to port my changes, so here I am trying to contribute back.

You are talking about added complexity because of criteria nested DTOs ? I very frankly think the code is very clean, (much better than my first try without this feature) and would really hope we would keep it.
Changes as you can see are very minimal (and some of them are just there to make it easier to implement composite keys).

I good compromise may be to generate criteria only for entities that are filterable. And one can, in its blueprints, add a @Criteria annotation or have some code that sets it (like I do, for more cases).
I could also settle, if you really want to, for not splitting the functionality in two query/criteria services , (even though I like separation of concerns) and only keep the change of nested criteria, but I really need this, since I want to do filtering with composite keys. I find this functionality very useful for large datasets....

What do you guys think, please helping here :pray: ? @gzsombor @deepu105 .

[deepu105]

First of all, I don't get why people want blueprints into main generator. Blueprints are more scalable and more flexible and wouldn't pose any such restrictions for your vision. When I built the blueprint mechanism I was really hopping that we will convert even core features into blueprints and thats why I was always against merging vuejs blueprint into core. Having everything in the core is not scalable and only makes things more complicated for everyone. @pascalgrimaud @jdubois @jhipster/developers we really need to fix our strategy on this and promote new features to be blueprints rather than core features its definitely not scalable otherwise and causes so many headaches. IMO we should set a policy of strictly not adding any new feature to the core unless it adds value to majority of the end users

Now @yelhouti I genuinely see no much advantage in having composite keys as core feature vs blueprint, I don't know what was the reason you decided to add it to core but may be we can look at root cause of why you think this needs to be a core feature instead of a blueprint. If there is an issue that arises when using it as a blueprint, we should rather fix that instead of keep adding more stuff to core. I'm sorry if you already explained this but for my sake could you tell again why you think this is not good enough as a blueprint 🙏

[yelhouti]

@deepu105 I was on the road yesterday, sorry for the delay.

I don't know what was the reason you decided to add it to core but may be we can look at root cause of why you think this needs to be a core feature instead of a blueprint.

Here is why i started merging @jdubois requested it:

@yelhouti you've had more than 1,000 installs for your blueprint (see http://www.npm-stats.com/~packages/generator-jhipster-composite-key-server ) which is good for something this specific. https://github.com/jhipster/generator-jhipster/issues/7112#issuecomment-631487047

I totally agree with the approach of trying to have almost everything in blueprints. but compoiste keys are a little bit different: 1- All blueprints no matter if it's backend frontend... that want to add support for composite key would need to add the logic of computing them if there are not in the main generator. I, for example, had to do this for both our primeng blueprint (frontend) and the composite-key-server (backend) (maybe I should have externalized the logic and made it idempotent in a 3th blueprint and have all other blueprints call it, but this can't be done in a clean manner any way since it is intricate since it adds a field to the list of fields and is based on relationships...). 2- If the logic is coded differently between blueprints they might be conflicts (I faced that with my own blueprints, because i made some stupid choices).

I don't get why people want blueprints into main generator. to answer this please let me first define (I am sur you have other names for these) :

• override blueprints: where you have a copy of the main generator part you want ex: server + server-entity and you add all your changes
• patch blueprint: where you would add annotations on top of endpoints for fine grained permissions using js / regexp for example.

When you have an override blueprint, like the one for composite keys, you need to rebase it at each release of the generator if you want it to have the latest features added by other people on the main generator, merging your code in the main generator prevents future changes from breaking your logic.

Let me throw some stones here :p context.fields used to only contain real fields (defined in JDL...), now someone added something called transient fields, inside the list of fields and in all my blueprints I need to go and replace field.forEach with field.filter(f = !f.transient).forEach. instead we could have something like javaEntityFields which is much cleaner, that all only changes fields in the entity-server context (where transient is needed) plus as you said this should have been a blueprint feature.

this got me thinking, I will create another PR that only compute context.ids so that other blueprints can use it.

[deepu105]

I was going to suggest that. Why don't you first add the logic needed to compute composite keys into the generator base as a public API methods, with test cases, this way any blueprint, including yours, can use it and your blueprint won't be broken easily by changes in core.

When Julien agreed for this feature request he indeed was asking how complex this would be and from what I have seen this indeed is quite complex and none of the core team members showed any interest in maintaining this. So IMO it is better for the core project and for you if this continues to be a blueprint as the future of JHipster has to be blueprints and I'm going to do everything I can to drive it that way.

What we really need to do is focus more on blueprints and ensure there are no breaking changes for blueprints unless its in a major version. Even then we should ensure backwards compatibility as much as possible. I'm going to propose some things for JHipster 8 to make this possible.

So as a blueprint author, I'm really interested in learning what can be improved in core to make it easier for you to maintain blueprints

On Tue, 9 Mar 2021, 3:17 pm yelhouti, notifications@github.com wrote:

@deepu105 https://github.com/deepu105 I was on the road yesterday, sorry for the delay.

I don't know what was the reason you decided to add it to core but may be we can look at root cause of why you think this needs to be a core feature instead of a blueprint.

Here is why i started merging @jdubois https://github.com/jdubois requested it:

@yelhouti https://github.com/yelhouti you've had more than 1,000 installs for your blueprint (see http://www.npm-stats.com/~packages/generator-jhipster-composite-key-server ) which is good for something this specific.

7112 (comment)

https://github.com/jhipster/generator-jhipster/issues/7112#issuecomment-631487047

I totally agree with the approach of trying to have almost everything in blueprints. but compoiste keys are a little bit different: 1- All blueprints no matter if it's backend frontend... that want to add support for composite key would need to add the logic of computing them if there are not in the main generator. I, for example, had to do this for both our primeng blueprint (frontend) and the composite-key-server (backend) (maybe I should have externalized the logic and made it idempotent in a 3th blueprint and have all other blueprints call it, but this can't be done in a clean manner any way since it is intricate since it adds a field to the list of fields and is based on relationships...). 2- If the logic is coded differently between blueprints they might be conflicts (I faced that with my own blueprints, because i made some stupid choices).

I don't get why people want blueprints into main generator. to answer this please let me first define (I am sur you have other names for these) :

• override blueprints: where you have a copy of the main generator part you want ex: server + server-entity and you add all your changes
• patch blueprint: where you would add annotations on top of endpoints for fine grained permissions using js / regexp for example.

When you have an override blueprint, like the one for composite keys, you need to rebase it at each release of the generator if you want it to have the latest features added by other people on the main generator, merging your code in the main generator prevents future changes from breaking your logic.

Let me throw some stones here :p context.fields used to only contain real fields (defined in JDL...), now someone added something called transient fields, inside the list of fields and in all my blueprints I need to go and replace field.forEach with field.filter(f = !f.transient).forEach. instead we could have something like javaEntityFields which is much cleaner, that all only changes fields in the entity-server context (where transient is needed) plus as you said this should have been a blueprint feature.

this got me thinking, I will create another PR that only compute context.ids so that other blueprints can use it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/13133#issuecomment-793956212, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIOKFZ4VZ3L3LZSJ4A6NHLTCYUXRANCNFSM4UAMWDQA .

[yelhouti]

@deepu105 I will start working on that tomorrow.

none of the core team members showed any interest in maintaining this

I would gladly become core team member and maintain the feature if it is all what it takes

So as a blueprint author, I'm really interested in learning what can be improved in core to make it easier for you to maintain blueprints

Templates are easy to read but hard to customize and because of that I often find myself needing to rewrite the whole file, even if the only thing I want is edit a simple function. A way we could solve this rewrite file like this:

<%_ imports.forEach(imp => { %> imp <% }) _%>

<%_ entity.javaClass.annotations.forEach(ann => { %> ann <% }) _%>
public class <%= entity.javaClass.name %> implements Serializable {
<%- include classDefinition(entity.javaClass) %>

<%_ entity.javaClass.fields.forEach(field => { _%> <%- include serverField(field) %> <_% }) _%>

<%_ entity.javaClass.constructors.forEach(field => { _%> <%- include serverConstructor(constcutor) %> <_% }) _%>

<%_ entity.javaClass.methods.forEach(method => { _%> <%- include serverMethod(method) %> <_% }) _%>

where classDefintion is a template file that looks like:

<%= javaClass.modifier %> <%= javaClass.name %> <% if (javaClass.extends?.length) { %>extends  <%= javaClass.extends.join(' ') %> <% } %>(implements...) {

constructor has a list of (injectableParams, simpleParams, extraCode)

serverMethod looks like

<%- method.doc %>
<%-beforeMethode %>
<%- method.annoations.forEach() %>
<%- include methodDefinition(method) %> {
<%- include(method.templateName, method.themplateParams) %>
}

And have the logic of each method defined in a separate template.

Of coure it is much more unreadable, but it is: 1- much more composable (for example introducing readOnly feature, means we remove update/patch/delete from entityClass.methods, adding multiTenancy is adding a tenantId in the list of fields...), 2- easier to add features without breaking changes 3- easier to test

Note that no variable is used twice: we use serverFields instead of fields ... dto/clientFields instead of fields even if they are the same... If someone wants to add multiTenancy for example he will change entity => get serverFields from return fields to return [...fields, {name: tenantId....}]

Once this is done, the only allowed changes in templates are new small methods templates and/or missing includeBlocks if a placeHolder was forgotten (example beforeClass)

When switching languages to closer language (ex: java to kotlin/scala...)methodDefinition... can be redefined to match the language spec and everything else will keep working, for other language the entityTemplate would be remove in favor of structTemplate for example...

This of course requires a lot of effort, and I am not sure every one would like but this is how I would do it. @deepu105 Regret asking yet :stuck_out_tongue: ?

[deepu105]

I like your proposal at first glance. Though I have to spend some time thinking further and others like @mshima @murdos who also have done some work refactoring blueprint system might also have a say. Regardless making blueprints more robust can be a focus for v8 and we could even do a design sprint over zoom or something for that with everyone interested in it along with some core members. Wdyt @jhipster/developers

[mshima]

When taking about JHipster internals, we should try to split into:

• Generators structure
• Workflow
• Blueprint system
• Templating system
• Application structure

---

Generators Hierachy

More than 40 generators and base classes.

Workflow:

Design of the step by step that should be followed for us to have the application generated.

• Each priority should take care of it's own tasks.
• It should be well defined so the next priority should have everything it needs and try to not touch anything the past priorities are responsible for.

Blueprint system

It loads blueprints and executes them.

Templating system

The templates and it's context.

Application structure

• Generated application config
• Entities graph
  • It's a graph, so it's not possible to be completely linear. For this reason, at specific use cases we are using getters (instead of eager calculation methods), so we can guarantee the data will not be outdated when needed.

---

JHipster v7

We rewrote almost from the ground the Workflow and the Application structure. Just maintenance changes in the Generators Hierachy, Templating system and Blueprint system.

My opinion of what's next?

Blueprint system

Nothing to change.

• This is javascript, we are free to do anything. Common changes for 2 or more blueprints can be implemented by:
  • Mixin. eg: MyGenerator extends loadMyMixin(require('generator-jhipster/generators/server')
  • Plugin system. Concept eg: module.export.createGenerator = env => class MyBlueprint extends env.get('jhipster:custom-server');

Generators Hierachy and Templating system

Split them. A specific change would make much difference:

• Stop using the generator as context for ejs templates. This will greatly benefit blueprints.
  • Change every generator to stop extending GeneratorBase and rename it to TemplateContext, and pass a TemplateContext instance to ejs templates.
  • Stop using GeneratorBase as api for template will make a lot easier to customize.

Other idea:

• Consider converting generator-jhipster into a mono-repository, so each technology (java backend/angular/react/vue) could have it's own release cicle,
  • generator-jhipster would have a hard dependency on each main technology, but customizable by acceptsDependencies This could prevent the spring 2.4 problem that the migration took almost a month to finish. At least it gave me time to finish generator v5 migration =).
  • The developer (JHipster user) would have the choice to keep spring version while updating frontend or the way around.
  • Should be somewhat a release train. v8.1.0 contains java v1.0.1, angular v2.0.1, etc...

---

About the proposal.

• Could be useful for Java backend, but we should not forget that we have many languages.
• A big change like this would require de original developer to keep working on it until it's 100% ready. We don't have the man power for a one time PR merge. So we must be confident the original author will finish the feature and he understands all the complexities of our blueprint requirements.
• IMO it should be incremental.

[deepu105]

Good ideas, I like some of them, have different opinions on others and have some of my own ideas. Anyway its too early to discuss now and lets not spam others on this thread. Once v7 is out I'll officially kick off some discussions around this

On Wed, 10 Mar 2021, 6:34 pm Marcelo Shima, @.***> wrote:

When taking about JHipster internals, we should try to split into:

• Generators structure
• Workflow
• Blueprint system
• Templating system
• Application structure

---

Generators Hierachy

More than 40 generators and base classes. Workflow:

Design of the step by step that should be followed for us to have the application generated.

• Each priority should take care of it's own tasks.
• It should be well defined so the next priority should have everything it needs and try to not touch anything the past priorities are responsible for.

Blueprint system

It loads blueprints and executes them. Templating system

The templates and it's context. Application structure

• Generated application config
• Entities graph
  • It's a graph, so it's not possible to be completely linear. For this reason, at specific use cases we are using getters (instead of eager calculation methods), so we can guarantee the data will not be outdated when needed.

---

JHipster v7

We rewrote almost from the ground the Workflow and the Application structure. Just maintenance changes in the Generators Hierachy, Templating system and Blueprint system. My opinion of what's next? Blueprint system

Nothing to change.

• This is javascript, we are free to do anything. Common changes for 2 or more blueprints can be implemented by:
  • Mixin. eg: MyGenerator extends loadMyMixin(require('generator-jhipster/generators/server')
  • Plugin system. Concept eg: module.export.createGenerator = env => class MyBlueprint extends env.get('jhipster:custom-server');

Generators Hierachy and Templating system

Split them. A specific change would make much difference:

• Stop using the generator as context for ejs templates. This will greatly benefit blueprints.
  • Change every generator to stop extending GeneratorBase and rename it to TemplateContext, and pass a TemplateContext instance to ejs templates.
  • Stop using GeneratorBase as api for template will make a lot easier to customize.

Other idea:

• Consider converting generator-jhipster into a mono-repository, so each technology (java backend/angular/react/vue) could have it's own release cicle,
  • generator-jhipster would have a hard dependency on each main technology, but customizable by acceptsDependencies This could prevent the spring 2.4 problem that the migration took almost a month to finish. At least it gave me time to finish generator v5 migration =).
  • The developer (JHipster user) would have the choice to keep spring version while updating frontend or the way around.
  • Should be somewhat a release train. v8.1.0 contains java v1.0.1, angular v2.0.1, etc...

---

About the proposal.

• Could be useful for Java backend, but we should not forget that we have many languages.
• A big change like this would require de original developer to keep working on it until it's 100% ready. We don't have the man power for a one time PR merge. So we must be confident the original author will finish the feature and he understands all the complexities of our blueprint requirements.
• IMO it should be incremental.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/13133#issuecomment-795790621, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIOKF4DX66W7ZC4I5XWONDTC6UTDANCNFSM4UAMWDQA .

[yelhouti]

@deepu105 I have been trying for hours to make my changes work without changing the current code two much but I just can't. When @mshima tried to implement composite ids "Incremental" he wrote a lot of code and added a lot of variables that are just blocking me...

examples: primaryKey.fields contains own and derived fields which works well for oneToOne but you don't want that when the key is composite.
primaryKey.derivedFields which we don't use since it only make code more complicated
primaryKey.ownFields which we call primaryKey.fields
Automatically adding the list of derived fields to the list of fields which breaks all our logic...

We now have a half baked feature that is blocking implementation, because when you don't see the bigger picture with all what you will need, I end up messing up stuff.

I would like to note that I spent weeks if not months working on this, and having not included in v7 even though I was told it would be if it's easy to maintain (which it is) was really upsetting, but finding out that the existing changes all over the place totally broke my blueprints, in an almost in recoverable fashion is just...

Please reconsider as if I don't merge my corrections the main generator we will end up with a lot of code that was added to implement the feature and that will end up: 1- breaking it 2- a nightmare to maintain since it introduce unused features + contain logical errors (in case of composite, works well for oneToOne)

[deepu105]

What do you propose as change here to make it work?

On Thu, 11 Mar 2021, 6:35 am yelhouti, @.***> wrote:

@deepu105 https://github.com/deepu105 I have been trying for hours to make my changes work without changing the current code two much but I just can't. When @mshima https://github.com/mshima tried to implement composite ids "Incremental" he wrote a lot of code and added a lot of variables that are just blocking me...

examples: primaryKey.fields contains own and derived fields which works well for oneToOne but you don't want that when the key is composite. primaryKey.derivedFields which we don't use since it only make code more complicated primaryKey.ownFields which we call primaryKey.fields Automatically adding the list of derived fields to the list of fields which breaks all our logic...

We now have a half baked feature that is blocking implementation, because when you don't see the bigger picture with all what you will need, I end up messing up stuff.

I would like to note that I spent weeks if not months working on this, and having not included in v7 even though I was told it would be if it's easy to maintain (which it is) was really upsetting, but finding out that the existing changes all over the place totally broke my blueprints, in an almost in recoverable fashion is just...

Please reconsider as if I don't merge my corrections the main generator we will end up with a lot of code that was added to implement the feature and that will end up: 1- breaking it 2- a nightmare to maintain since it introduce unused features + contain logical errors (in case of composite, works well for oneToOne)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/13133#issuecomment-796469270, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIOKFYDCHHCGCRD4U7NPQLTDBJCPANCNFSM4UAMWDQA .

[yelhouti]

@deepu105 I don't understand what the "it" refers to.

If it's composite keys, here is a PR: https://github.com/jhipster/generator-jhipster/pull/12819 I am willing to change anything you don't like to finally have support.

If you really don't want the support, I would remove from primaryKey all the added fields (relationships, fields, ownFields, derviedFeilds, references...) and all there uses in templates (by reverting to what it was before it was introduced OR by change it so that it works with primaryKey.ids.

If you are talking about something else, please ask again XD .

EDIT: I think also that how long I have been working on this proves that I am willing to maintain it and review code related to it. So please choose option 1.

[deepu105]

The PR is too huge and is already reviewed by multiple people so I'm not gonna review it again. I was talking about your proposal to improve what we have currently on the core (no templates, just core) that could make it easier for the composite key blueprints

On Thu, 11 Mar 2021, 3:23 pm yelhouti, @.***> wrote:

@deepu105 https://github.com/deepu105 I don't understand what the "it" refers to.

If it's composite keys, here is a PR: #12819 https://github.com/jhipster/generator-jhipster/pull/12819 I am willing to change anything you don't like to finally have support.

If you really don't want the support, I would remove from primaryKey all the added fields (relationships, fields, ownFields, derviedFeilds, references...) and all there uses in templates (by reverting to what it was before it was introduced OR by change it so that it works with primaryKey.ids.

If you are talking about something else, please ask again XD .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/13133#issuecomment-796772442, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIOKF4MFTB7DKNOPINGV73TDDG45ANCNFSM4UAMWDQA .

[yelhouti]

@deepu105 well templates use the broken variables, plus once these are released in 7.0.0 they will become public api and much harder to change. Unless you are willing to remove/replace them they will cause problems in the future.

I suggest to rewrite the parts that use them to use primaryKey.ids instead (without adding support for composite keys), but taking it into account when possible. I can do that, but I will have to change templates, and i can't promesse that PR will be 10 times smaller, it will basically be a refactoring and therefore will touch a lot of files...

What do you think ?

[deepu105]

Ok can you do a new PR for that and it should only contain this and no other unrelated changes/optimizations and definitely not the composite id feature. Just changes that would be needed for a blueprint to work easier

On Thu, 11 Mar 2021, 9:35 pm yelhouti, @.***> wrote:

@deepu105 https://github.com/deepu105 well templates use the broken variables, plus once these are released in 7.0.0 they will become public api and much harder to change. Unless you are willing to remove/replace them they will cause problems in the future.

I suggest to rewrite the parts that use them to use primaryKey.ids instead (without adding support for composite keys), but taking it into account when possible. I can do that, but I will have to change templates, and i can't promesse that PR will be 10 times smaller, it will basically be a refactoring and therefore will touch a lot of files...

What do you think ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/13133#issuecomment-797031312, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIOKFYUSBQJAQWTVZUGHA3TDESR3ANCNFSM4UAMWDQA .

[yelhouti]

now that the dust has settled, I will update the proposal above to reflect more what is wanted.