Composite primary key support

[yelhouti]

Overview of the issue

Hi guys, I'm working on the support of composite primary keys in jhipster. the goal is to allow use cases where we for complex table relationships... This thread will help follow advancement of the work and discuss design choices that need to be made. I'll try to put different questions here, and discuss them in the comments:

1. How to specify primary keys in the entity.json
2. How to handle urls: GET /id1/id2/id3, vs /id1-id2-id3 ...
3. How to handle Create vs Update (the keys is always specified since there is no AUTOINCREMENT...)

Motivation for or Use Case

When we want a many to many relationship with additional fields or more than two tables we need a lot of boiler plate code.

Reproduce the error

Related issues

Suggest a Fix

JHipster Version(s)

Environment and Tools

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

git version 2.14.3 (Apple Git-98)

node: v6.10.3

npm: 5.6.0

bower: 1.7.7

yeoman: 2.0.0

Docker version 17.12.0-ce, build c97c6d6

docker-compose version 1.18.0, build 8dd22a9

4.14

JHipster configuration

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

.yo-rc.json file

{
  "generator-jhipster": {
    "promptValues": {
      "packageName": "com.xxx.yyy.zzz",
      "nativeLanguage": "en"
    },
    "jhipsterVersion": "4.14.0",
    "baseName": "zzz",
    "packageName": "com.xxx.yyy.zzz",
    "packageFolder": "com/xxx/yyy/zzz",
    "serverPort": "8081",
    "authenticationType": "oauth2",
    "cacheProvider": "hazelcast",
    "enableHibernateCache": true,
    "websocket": false,
    "databaseType": "sql",
    "devDatabaseType": "mysql",
    "prodDatabaseType": "mysql",
    "searchEngine": false,
    "messageBroker": false,
    "serviceDiscoveryType": "eureka",
    "buildTool": "gradle",
    "enableSocialSignIn": false,
    "enableSwaggerCodegen": false,
    "jwtSecretKey": "replaced-by-jhipster-info",
    "enableTranslation": true,
    "applicationType": "microservice",
    "testFrameworks": [],
    "jhiPrefix": "jhi",
    "nativeLanguage": "en",
    "languages": [
      "en",
      "fr"
    ],
    "clientPackageManager": "npm",
    "skipClient": true,
    "skipUserManagement": true
  }
}

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

I propose to add something like this in normal json file, I will also modify the example as the discution go further:

{
    "fluentMethods": true,
    "relationships": [
        {
            "relationshipType": "many-to-one",
            "relationshipValidateRules": "required",
            "relationshipName": "business",
            "otherEntityName": "business",
            "otherEntityField": "name"
        },
        {
            "relationshipType": "many-to-one",
            "relationshipValidateRules": "required",
            "relationshipName": "basicIndex",
            "otherEntityName": "basicIndex",
            "otherEntityField": "name"
        }
    ],
    "fields": [
        {
            "fieldName": "year",
            "fieldType": "Integer",
            "fieldValidateRules": [
                "required"
            ]
        },
        {
            "fieldName": "month",
            "fieldType": "Integer",
            "fieldValidateRules": [
                "min",
                "max"
            ],
            "fieldValidateRulesMin": 1,
            "fieldValidateRulesMax": 12
        },
        {
            "fieldName": "value",
            "fieldType": "Integer",
            "fieldValidateRules": [
                "required",
                "min"
            ],
            "fieldValidateRulesMin": 0
        }
    ],
    "id": [
        {
            "attributeName": "businessId",
            "columnName": "business_id"
        },
        {
            "attributeName": "basicIndexId",
            "columnName": "basic_index_id"
        },
        {
            "attributeName": "year",
            "columnName": "year"
        }
    ],
    "changelogDate": "20180210195726",
    "entityTableName": "business_basic_index",
    "dto": "mapstruct",
    "pagination": "no",
    "service": "serviceImpl",
    "jpaMetamodelFiltering": true,
    "microserviceName": "businessman"
}

This is clearly related to JDL, but JDL support will be added later and will depend on the new entity.json format

Browsers and Operating System

OSx and not relevant

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

Example repo for what the result should be, all suggestions are welcome: https://github.com/yelhouti/jhipster-composite-key

[yelhouti]

1. I went with what is in the example, but I soon had to guess what was the type of the primary fields from each relationship, (since now we can have complex relationships ex: A<->AB<->B then AB<-> ABC <->C) then it's not possible to know the types of the primary keys of AB in ABC and having ABC.json independent from the other files without define the types of A and B primary keys in this file. I also suggest to put A, and B in different lines in the id field of the .json file (one entry per table instead of 1 per relashionship), and define the type in the id entry. EDIT: current relashionship entries has an "otherEntityName" feild which doesn't specify what the ID is (how many feilds or of which type they are). I circumvented that by specifying this information in the ID field I added, but now I need that information when building EntityClass to know the column used for the Join. I'm thinking about:
   • removing the ID field I added.
   • class fields can have property (primaryKey: true)
   • adding otherEntityId fields for each relationship if needed (!= id) ()
   • adding primaryKey in relationship (like in class field)
2. I went for /id1/id2/id3, because it will avoid parsing problems (ex: id contains "-" or the other separator)
3. we can query the DB to check if we are doing an update or a create, I would appreciate suggestions on this matter. (2 db querys is not the best way to go in my opinion)

[jdubois]

I agree this is a feature many people would like, at least to reverse-engineer an existing database. However, we have far too many opened issues at the moment (42!!!), and we are working on JHipster 5 - there is no way we have enough time to review or merge your code at the moment, I prefer to warn you now.

[yelhouti]

I understand, thanks for the heads up. all I need is help for the spec, the merge will come when it will.

[gmarziou]

I'm not a big fan of /id1/id2/id3 URL, it is not very RESTful, usually sub routes are used for nested resources or variants. ',' separator seems a better choice, see https://stackoverflow.com/questions/21663635/url-to-rest-resource-with-a-composite-id

Still I have no idea how springfox will produce the swagger JSON file, have you tried ?

[yelhouti]

@gmarziou since they are in the same controller the are groupped:

[gmarziou]

From JPA perspective, do you plan to use @Embeddable and @Embedded? This is rather common usage for this case.

[gmarziou]

This works in swagger because you use nested routes but id is not materialized this way.

[yelhouti]

@gmarziou yes I did use @Embeddable and @Embedded, it kind of conflicts with @ManyToOne ... I solved it like this: https://stackoverflow.com/questions/29952386/embedded-id-and-repeated-column-in-mapping-for-entity-exception (when using DTO's there is no conflict), but I don't know how it will behave when it's not the case.

[gmarziou]

So if you materialize the id an an embedded class, the default "otherEntityName" field could be the toString() method of the embedded, this way developer could easily customize it, not sure how to expose this to frontend though.

[DanielFran]

I agree with @gmarziou about the URL, you should use URL mapping with matrix variables.

http://www.baeldung.com/spring-mvc-matrix-variables

https://books.google.pt/books?id=kcSNCgAAQBAJ&pg=PA114&lpg=PA114&dq=spring+rest+url+with+semi-colon&source=bl&ots=HrRU85ch4L&sig=v38eeIMD_JS01gv_y4hO1LlSbvw&sa=X&ved=0ahUKEwiUopLswKPZAhVL8RQKHWreBloQ6AEIbTAH#v=onepage&q=spring%20rest%20url%20with%20semi-colon&f=false

https://books.google.pt/books?id=GqDcDgAAQBAJ&pg=PA746&lpg=PA746&dq=spring+rest+url+with+semi-colon&source=bl&ots=4ytff1j9R0&sig=p4iwdIl2aZCxyT2rp5Miv4YyEOk&sa=X&ved=0ahUKEwiUopLswKPZAhVL8RQKHWreBloQ6AEIcjAI#v=onepage&q=spring%20rest%20url%20with%20semi-colon&f=false

[DanielFran]

About QueryService, when implementing specification for fields that are part of the composite key, you will need to use for example:

if (criteria.getBusinessId() != null) {
    specification = specification.and(buildReferringEntitySpecification(criteria.getBusinessId(), 
                       BusinessBasicIndex _.id, BusinessBasicIndexId_.businessId));
            }

[DanielFran]

Looking are your code example:

    @EmbeddedId
    @AttributeOverrides({
        @AttributeOverride(name = "businessId", column = @Column(name = "business_id", nullable = false)),
        @AttributeOverride(name = "basicIndexId", column = @Column(name = "basic_index_id", nullable = false)),
        @AttributeOverride(name = "year", column = @Column(name = "jhi_year", nullable = false))
    })
    private BusinessBasicIndexId id;

The use of @AttributeOverrides is not needed since the column names are the same defined in the class.

[deepu105]

@jhipster/developers anyone in favor of supporting this feature request? IMO we already have too much stuff to take care of and I don't like to see @yelhouti spending effort to do this and that end up being stale PR. @yelhouti maybe you should consider doing it as a module so that you don't have to depend on us to merge it and you have more freedom

This feature is more suitable to be built as a JHipster module so that it can be evaluated first. If the module ends up being very popular we could consider integrating it into the main project here. Please refer the documentation on how to build modules and look at some of the existing modules for inspiration. Reach out to us if you need any help like clarification on how the module system works, adding/exposing new methods for the API etc. You can use the JHipster module generator to scaffold a module.

[deepu105]

@yelhouti i'm closing the issue as we wont be doing anything on this, we can still continue to discuss here

[yelhouti]

Well, IMHO this id the kind of features that jhipster can't afford not having, all projects using SQL at some point use non Autogenerated Ids, and composite keys... also I'm alawya on gitter and many poeple have requested this. I'll keep working on it, and discuss it with other members, but if you don't think it's worth it feel free to not participte...

[jdubois]

Yes we need this, but we have far too much work at the moment, and we have more than 40 opened tickets... We mostly do this on our free time, and we can't do everything. For me composite keys are useful for people who already have a database (otherwise they are always a bad choice), and this is not our core focus as JHipster is supposed to be an "application starter", so it's for new applications - even if I understand some people want to do this, of course!

[gmarziou]

I think it's a good feature but I also think that any change dealing with entity relationships is always painful to implement as it impacts many things in the generator. I have discussed on gitter with @yelhouti and we think the best for him is to expose his ideas through a sample app to collect feedback before working on the generator or a module.

[DanielFran]

If you see in stackoverflow, lot of people use jhipster to create the project + entities and then adapt the code to use entities with composite keys! This is indeed what I do.... And i think this is definitly a missing part of the core of jhipster-generator, at least I see that this functionality will permite adoption to a new community to continue growing up using jhipster.

[deepu105]

Well, that's a bit exaggerated. I have worked with a lot of SQL projects and have audited a lot more and have seen composite keys being used in less than 10% of them and if you have a designed a proper entity model then generated primaries keys are more than enough and IMO are even better. But of course if you prefer to use composite keys that's up to you and your use case but I wouldn't go on to say that its the holy grail for JHipster. I agree with what @gmarziou have suggested.

[yelhouti]

never said composite keys are the "holy grail for JHipster", jhipster is much more than this feature, but I still think it's must have, I also agree with @gmarziou so we will follow this route I guess, and @deepu105 thank you for the feedback :) and all your great work on the project

[yelhouti]

for Information, here is a repo with the proposed changes: https://github.com/yelhouti/jhipster-composite-key

[DanielFran]

I take a look and seems OK to me, but found small issues and I will push the fixes!

Note: I spend some time to implement a version with Matrix Variables, but now I conclude it is not a good solution because we would have URL with someting like: @GetMapping("/business-basic-indices/{businessId}") @Timed public ResponseEntity getBusinessBasicIndex(@PathVariable Long businessId, @MatrixVariable Long basicIndexId, @MatrixVariable Integer year) {

Since it is mandatory to have Matrix Variables (basicIndexId and year) dependent on a Path (businessId), seems not good looking!

@yelhouti , @gmarziou , what do you think?

[gmarziou]

First, I haven't implemented such thing so I can't provide any real experience feedback.

My intuition is that we should avoid using '/' in URL because it has a very special meaning in server (REST nested resources) and in client (Angular routes, maybe same for React).

I haven't played a lot with MatrixVariable but to me the controller methods should use the embedded type as argument and this could be done with custom argument resolvers.

@GetMapping("/business-basic-indices/{businessId}")
public ResponseEntity getBusinessBasicIndex(@PathVariable BusinessBasicIndexId id) {

It would be the job of the argument resolver to extract the composite key from request path maybe using methods in DTO Mapper or in BusinessBasicIndexId.

Few things are still not clear to me: JSON structure in REST API and impact on the client model. Should the id in JSON be flattened or an embedded object?

[yelhouti]

@DanielFran I emplemented something with the id joined by ',' so: id1,id2,id3 I didn't push it yet, it also makes urls shorter so I kind of prefer it this way. what do you think? I also like the idea of using the custom argument resolver, for now I just o it like this:

    @GetMapping("/business-basic-indices/{businessId},{basicIndexId},{year},{month}")
    @Timed
    public ResponseEntity<BusinessBasicIndexDTO> getBusinessBasicIndex(@PathVariable Long businessId, @PathVariable Long basicIndexId, @PathVariable Integer year, @PathVariable Integer month) {
        BusinessBasicIndexId id = new BusinessBasicIndexId(businessId, basicIndexId, year, month);
        log.debug("REST request to get BusinessBasicIndex : {}", id);
        BusinessBasicIndexDTO businessBasicIndexDTO = businessBasicIndexService.findOne(id);
        return ResponseUtil.wrapOrNotFound(Optional.ofNullable(businessBasicIndexDTO));
    }

The code this way is easily generated PS: in this case the month is also part of the Id

[DanielFran]

@gmarziou I agree with you that we should not use "/" in the url!

@yelhouti This seems good to me! I have some doubt if it should be used ',' or ';' instead. If we compare with Matrix Variables, ';' is used to separate different variables and ',' is used for a list of variable values.

Note: Since you are using here DTOs, you should not use BusinessBasicIndexId in the resource. You should use the existing BusinessBasicIndexDTO or create a specific BusinessBasicIndexIdDTO.

[yelhouti]

@DanielFran since I don't use the variable name, I decided to use ',' we can change of course. For the Id part, I used the Id, beacuse later in the JPA: JpaRepository<BusinessBasicIndex, BusinessBasicIndexId> You need to specify an Id, and all methods like findOne expect an Id Also there is no need to have a DTO for the Id, since Id is a POJO...

[DanielFran]

@yelhouti Here what I implemented:

Resource:

    @GetMapping("/business-basic-indices/{businessId}/{basicIndexId}/{year}")
    @Timed
    public ResponseEntity<BusinessBasicIndexDTO> getBusinessBasicIndex(@PathVariable Long businessId, @PathVariable Long basicIndexId, @PathVariable Integer year) {
        log.debug("REST request to get BusinessBasicIndex : {}", businessId, basicIndexId, year);
        BusinessBasicIndexDTO businessBasicIndexDTO = businessBasicIndexService.findOne(businessId, basicIndexId, year);
        return ResponseUtil.wrapOrNotFound(Optional.ofNullable(businessBasicIndexDTO));
    }

Service implementation:

    @Override
    @Transactional(readOnly = true)
    public BusinessBasicIndexDTO findOne(Long businessId, Long basicIndexId, Integer year) {
        log.debug("Request to get BusinessBasicIndex : {}", businessId, basicIndexId, year);
        BusinessBasicIndexId id = new BusinessBasicIndexId(businessId, basicIndexId, year);
        BusinessBasicIndex businessBasicIndex = businessBasicIndexRepository.findOne(id);
        return businessBasicIndexMapper.toDto(businessBasicIndex);
    }

I believe that if we use DTO we should not use domain classes in resource.

About the validation in create or update entity in resource, I validate if the entity exists implementing the exists functionality in service that call the exists crud function.

I also reimplemented the tests without using the elastic search...

[yelhouti]

@DanielFran there was a problem with teh copy paste, could you try again, also, could we use gitter, for this broder discutuons to avoid poluting the thread, and keep here only general info when we agree?

[DanielFran]

@yelhouti Sorry, my copy&paste is strange!

Ok to use gitter!

[gmarziou]

I just edited your post, enclose your code within ~~~java and ~~~

[yelhouti]

@gmarziou thanks for the tip, @DanielFran I see what you did there, prsonnaly I see no added value, in my opinion it will only comlicate the generators code, if more poeple agree with you we can rediscuss this :)

[DanielFran]

@yelhouti I pushed some changes!

https://github.com/yelhouti/jhipster-composite-key/pull/1

[yelhouti]

@DanielFran could you try again without reformating/reordering the code, so I can check the real code changes? would you like to that before or after I push all my recent changes?

[DanielFran]

@yelhouti I added comments to my code...

[brunnels]

looks like customization to the .jhipster entity json and blueprint is the way forward with this. I'm working on something if anyone wants to help.

[deepu105]

@brunnels yes that would be a great start

[yelhouti]

@brunnels I can help, but first I think we should work on repo like, this: https://github.com/yelhouti/jhipster-composite-key to show how we expect the final result to be. Once we agree we code the blueprint. Also, I think that at the same time we could work on Ids, with different type (ex: String...) You can contact me on gitter to be more efficient.

[MathieuAA]

Such an interesting feature, good luck guys! I'll be sure to update the JDL once this is done. One question though, why a blueprint? 

[yelhouti]

@MathieuAA I agree, I would prefer having this in the main project.

[MathieuAA]

Unless I'm wrong about blueprints (which I may be), this should be on the main project as this is a "small thing". Not to minimize the impact of the work, but I don't see how this can't be in the main project... Or a module at least.Le 19 juil. 2018 14:17, yelhouti notifications@github.com a écrit :@MathieuAA I agree, I would prefer having this in the main project.

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

[yelhouti]

If we want end to end support (JDL -> .json -> code) support, we indeed must have this in the main project, or else we would have to modify json manually...

[deepu105]

@MathieuAA the problem is this is a PITA to maintain as it will complicate the templates, especially the angular and react ones a lot and I think its better to start as a blueprint. If the blueprint becomes popular we can always integrate it into generator. But for now we have enough complexity IMO

[MathieuAA]

Exactly. I have an idea for this. Le 19 juil. 2018 14:42, yelhouti notifications@github.com a écrit :If we want end to end support (JDL -> .json -> code) support, we indeed must have this in the main project, or else we would have to modify json manually...

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

[yelhouti]

@deepu105 I understand your hesitation, the problem is that if we don't have JDL support it would a PITA for all the users to use it. Also I don't think the fornt end will get complicated as it's equivalent of DTOs...

[yelhouti]

@MathieuAA could you check the gitter example please: https://gitter.im/jhipster/generator-jhipster

[MathieuAA]

Almost good enough for me... But if it's a blueprint, I need to find a way to support what the blueprint enables.For instance, if a user doesn't use the blueprint, how can I know that the being-parsed JDL is valid and doesn't use something only logical and accepted for a blueprint?I have an idea about that, but this needs testing.

[DanielFran]

@brunnels @yelhouti I can help also if needed.

[yelhouti]

@DanielFran I'll invite ou on gitter

[jiangzhixiao]

i think this is great feature

[yelhouti]

A working and tested example is finally ready here. I will start working the generator if it's ok with everyone, the code of the generator will (for now) be in this branch of my fork. any help is welcome, but please before starting say on which part you will work so we don't step on each others toes. Thanks in advance.