Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding MapsId to one-to-one mapping in jdl #7060

Closed
1 task done
tibistibi opened this issue Jan 30, 2018 · 21 comments · Fixed by #8685
Closed
1 task done

adding MapsId to one-to-one mapping in jdl #7060

tibistibi opened this issue Jan 30, 2018 · 21 comments · Fixed by #8685
Assignees
Milestone

Comments

@tibistibi
Copy link
Contributor

tibistibi commented Jan 30, 2018

Overview of the issue

With jdl it is possible to generate a one-to-one mapping. The code will look like this:

public class Client  {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
@OneToOne
@JoinColumn(unique = true)
private User user;
 ....
}

in line with the documentation here: http://www.jhipster.tech/tips/022_tip_registering_user_with_additional_information.html
it would be a performance improvement when the class would reuse the id of the user. like this:

public class Client  {
@Id
private Long id;
@OneToOne
@JoinColumn(unique = true)
@MapsId
private User user;
 ....
}
Motivation for or Use Case

This improvement is for performance because there is only one index created and the database will have one less column. it should also be more efficient for hibernate.

When changing this by hand and generate the corresponding changelog with liquibase take care that there is a bug there. a working changeset is:

	<modifyDataType columnName="id" newDataType="int" tableName="client" />
	<dropPrimaryKey tableName="client" />
	<dropColumn columnName="id" tableName="client" />
	<addPrimaryKey columnNames="user_id" constraintName="clientPK" tableName="client" />
	<addNotNullConstraint columnDataType="bigint" columnName="user_id" tableName="client" />

i think this is a minor improvment. so when it is to complicated don't do it. programmers can copy the above ;)

  • Checking this box is mandatory (this is just to show you read everything)
@tibistibi
Copy link
Contributor Author

tibistibi commented Jan 30, 2018

to make both mysql work and h2 i needed to change it to:

<changeSet author="tibi (generated)" id="1517321558494-7" dbms="mysql">
	<modifyDataType columnName="id" newDataType="int" tableName="client" />
</changeSet>
<changeSet author="tibi (generated)" id="1517321558494-8">
	<addNotNullConstraint columnName="user_id" tableName="client" columnDataType="bigint" />
	<dropPrimaryKey tableName="client" />
	<dropColumn columnName="id" tableName="client" />
	<addPrimaryKey columnNames="user_id" constraintName="clientPK" tableName="client" />
	<addNotNullConstraint columnDataType="bigint" columnName="user_id" tableName="client" />
</changeSet>

@deepu105
Copy link
Member

@tibistibi do you want to do a PR?

@tibistibi
Copy link
Contributor Author

in march i have some time to look into this, but i have not jet looked at how the generator of jhipster works so not sure if i can get it done within the time i have. any pointers to documentation about that would be great.

@jdubois
Copy link
Member

jdubois commented Feb 14, 2018

You mean about our template system? Have a look at http://yeoman.io/ and http://ejs.co/ - but what we do is so simple you don't really need to read the documentation (in fact I've never read http://ejs.co/ - maybe I should do it one day!!)

@tibistibi
Copy link
Contributor Author

ok sounds good. i put it on my list. but my clients will kill me if you do it now ;)

@Dufgui
Copy link
Contributor

Dufgui commented May 13, 2018

Hello, I take a look on this old issue.
Just two questions :

  • How to deal with nullable relation (not required relation) ? I think it's mandatory to activate this optimization no ? (already dev in my test dev branch)
  • How to deal with multiple one to one on the same owner ? We can activate it, when we have just one relation (required?) one to one by entity, no ? (not yet dev)

@jdubois
Copy link
Member

jdubois commented May 14, 2018

Thanks @Dufgui - I didn't spend enough time on this, and this has been opened for way too long!! Now I really would like to have this configuration, but I have no idea how to answer your questions - that depends on the use case, of course. So the problem is that this might make things much more complex, and I don't think one-to-one relationships are very popular...
In that case I would keep the current code, as it allows more options, and as the performance issue shouldn't be very big.
@Dufgui as you worked on this (I saw your fork), maybe you have some solution or idea?

@tibistibi
Copy link
Contributor Author

tibistibi commented May 14, 2018

the idea is that it is used in a B extends A situation. like with users. You have a base user and then you have clients and employee's which are extensions on the base users.
not for one-to-one relations like B has an A like a users has an address.

so i would say

  1. nullable should not be an option. it is meant to be an id so a null id would be a strange situation
  2. multiple one-to-one I would also not support. the @mapsid that it maps an id. usually you have only one id.

@Dufgui
Copy link
Contributor

Dufgui commented May 14, 2018

Thanks @tibistibi It’s confirm what I am thinking. So I finish the dev and you will see if it’s enough simple @jdubois

@jdubois
Copy link
Member

jdubois commented May 26, 2018

OK @tibistibi so if I understand well that means we would be a bit less generic (only 1 one-to-one relationship), but that's reasonable (doing 2 1-to-1 relationships is starting to be really strange), and we gain better performance and code for most people.

Now, what could be even more perfect, is to have an option asking people if they want a MapsId (saying that this would limit them to one 1-to-1 relationship but they'll have better performance), or a "normal" one-to-one relationship.

Anyway, for MapsId, don't forget to add a check to forbid the use of a second one-to-one relationship, with a clear explanation message - that would be quite complex to understand for end-users.

@binakot
Copy link

binakot commented Jul 8, 2018

I think most of these requests are around of adding extra fields to already existing User entity.
Few days ago I worked on this problem too. It's a lot of template code.
I found few helpful links: https://stackoverflow.com/questions/36289146/add-attributes-to-user-entity-in-jhipster and https://www.jhipster.tech/tips/022_tip_registering_user_with_additional_information.html

I don't like the idea to add one more entity and mapping it one to one with User entity. I added my extra field (phone number) directly to User and add all required code (get/set, dto, service, tests, etc).

What do you think about the possibility to add extra fields while generating application? Some question about additional fields when user is generating application with JHipster?

@MathieuAA
Copy link
Member

@binakot Adding fields to the User entity is an avoidable problem because the User entity is generated during application generation. If you want to add more fields, or to make any kind of modification (adding, modifying, removing things) I suggest you use the "side-by-side" approach.
I see you don't like composing with the User entity, have you thought of using plain' old inheritance?
You create an "extended" User (you can call it whatever you want) where you add everything you want to (fields, methods, options, etc.) and only modify this extended class without changing the User class.

It's better than adding questions in my opinion.

@binakot
Copy link

binakot commented Jul 8, 2018

@MathieuAA Hmm, just using inheritance is may be the best way. I just need to extend User class and replace all User class using with my child class. I will try to work around this way.

@jdubois jdubois added the $100 https://www.jhipster.tech/bug-bounties/ label Jul 23, 2018
@pmverma
Copy link
Member

pmverma commented Sep 18, 2018

Is anyone looking into this? I can do a PR if nobody is working on this?

@jdubois
Copy link
Member

jdubois commented Sep 18, 2018

@pmverma I'm pretty sure nobody works on this, I'll assign the ticket to you if you're fine with it (for this I'll add you to a JHipster group, you'll get an email)

@pmverma
Copy link
Member

pmverma commented Sep 18, 2018

Thanks @jdubois I am fine. Please assign.

@jdubois
Copy link
Member

jdubois commented Sep 18, 2018

@pmverma you need to accept the invitation to the org first :-)

@pmverma
Copy link
Member

pmverma commented Sep 18, 2018

@jdubois Thanks for the invitation. I have just accepted.

@jdubois
Copy link
Member

jdubois commented Oct 19, 2018

@pmverma no news since one month, will you work on this? Otherwise this has been opened for very long, we also added a bug bounty to it.... so at some point it's time to give up, we just don't have the bandwidth to do everything

@pmverma
Copy link
Member

pmverma commented Oct 22, 2018

@jdubois I am going to have some free time in this weekend and I will do the PR.

@jdubois
Copy link
Member

jdubois commented Oct 22, 2018

Awesome, thanks a lot!

@jdubois jdubois added this to the 5.8.1 milestone Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants