[cbornet]

Because fluent is much nicer !

[jdubois]

The entity is one of the core JHipster object, can we discuss this before? I'm not familiar with having a "fluent" setter on a domain object - I see the point, of course, but I've never seen those used on JPA entities, and wonder if there are any drawbacks or issues. Besides, I'm not sure everyone will want those (as I've never seen those used for domain objects), so that would mean dead code for a lot of people. Of course, if there are only advantages, we could also evangelize people to use those more!

[deepu105]

Either we should use normal setters or fluent setters, having both might be lot of dead code. I don't see any downside in having fluent setters that follow the same naming convention like below

class Foo {

  private String bar;

  public String getBar() {
    return bar;
  }

  public Foo setBar(String bar) {
    this.bar = bar;
    return this;
  }
}

[cbornet]

I think setBar can lead to issues with JavaBean reflection so bar(String bar) would be better.

[cbornet]

I thought hibernate would complain if it didn't have the void setBar() method for its reflection but it seems to be OK so maybe we can remove it.

[deepu105]

@cbornet what about mapstruct does it play well with this syntax

[deepu105]

btw what issue do you foresee with JavaBean reflection?

[cbornet]

I don't know, something I read somewhere...
Anyway, we can test with setBar() and if it works, it's just a matter of preference between setBar() or bar().

[cbornet]

As for mapstruct, I didn't test directly but I guess that it was tested OK by CI.

[jdubois]

you need setBar otherwise you are not following the JavaBean convention. Speaking of this, you should also return void, so I'm really not sure we should do this.

[cbornet]

That's why I duplicated the method. But on in other hand, I removed the void setBar method and both Jackson and Hibernate seem to be fine with it.

[cbornet]

To show the interest of fluent setters, compare

Foo foo = new Foo();
foo.setMyString("foo");
foo.setMyString2("bar");
fooRepository.save(foo);

with

fooRepository.save( new Foo().myString("foo").myString2("bar") );

This is a simple example but it gets more and more interesting when structures get complex. Consider

Foo foo = new Foo();
foo.setMyString("foo");
foo.setMyString2("bar");
fooRepository.save(foo);

Bar bar = new Bar();
bar.setMystr("bar");
bar.setFoo(foo);
barRepository.save(bar);

vs

barRepository.save( new Bar()
    .foo(fooRepository.save(new Foo()
        .myString("foo1")
        .myString2("foo2")))
    .mystr("bar"));

With the fluent version, it almost reads as YAML !

Note: the repository save can probably be removed if the entities have CascadeType.ALL on their relationships.
@jdubois Is there a reason why we don't set the relationships as CascadeType.ALL by default ?

[jdubois]

• OK the fluent interface looks better, then I've never seen anyone using it -> that's why it worries me, and I wonder if there is a technical issue somewhere
• For CascadeType.ALL this is because of our Policy 1. Anyway here you have a different issue: you need to save() because that's a new entity, so it's non-managed. Otherwise it would work automatically (at least without the fluent interface, it works!!)

[cbornet]

I thought CascadeType.ALL (or persist) was used so that the entity is automatically added to the session. I need to check that again.

[cbornet]

If there is no JavaBean setter, Hibernate uses properties directly even if they are private (unlike standard JPA) and Jackson will both serialize and deserialize (as long as there is a getter).
Now for the other frameworks that use the domain (mongo, ES, cassandra, liquibase ...) , I don't know the impact of not having a Javabean setter but it seems the CI tests where good.

My opinion would be to have both void setBar() and bar() generated to avoid issues with libraries that expect to have the JavaBean setter. JavaBean reflection will just ignore the bar() method.

[cbornet]

In java.beans.Introspector

                       } else if (void.class.equals(resultType) && name.startsWith(SET_PREFIX)) {
                            // Simple setter
                            pd = new PropertyDescriptor(this.beanClass, name.substring(3), null, method);
                            if (throwsException(method, PropertyVetoException.class)) {
                                pd.setConstrained(true);
                            }
                        }

so the void result type is checked before adding the method to the PropertyDescriptor list.
Also a non-javabean method will just be ignored so it's safe to add them.

[jdubois]

• I'll let you have a look at CascadeType.ALL :-)
• Then we are going to have both setBar() and bar(), and once again maybe it can cause issues (JPA, MapStruct, Jackson... so many stuff use the domain object), and this is going to be dead code for a lot of people, so this is bad for them. At best this could be an option.

[cbornet]

I tested that

PropertyDescriptor descriptor = new PropertyDescriptor("bar", Foo.class);

fails with

java.beans.IntrospectionException: Method not found: setBar

If the void setBar method is not present

[cbornet]

It's not really dead code : it's code that people may not use (even if they should 😄 ) and it's in generated code so it's a bit like a library : you don't really need to go see the internal code often.

But if you prefer to do it as an option or module I understand. Please tell me which one in that case.

[cbornet]

This reminds me that I wanted to add adders and removers on relationships Set. So that could be part of the package.

[jdubois]

It's just that people keep telling me JHipster generates too much code, and that they don't understand what is being generated.
Of course, you could argue that if they don't understand the code, they shouldn't say there is too much code :-)

[cbornet]

So they want awesome features but no code. Like in magic ?

If you're ok, I'll do it as a silent option (no question asked) so that it can be configured in jhipster-uml/JDL

[deepu105]

--option might be good. So that people who knows and uses these stuff can
enable them when required

Thanks & regards,
Deepu
On 23 Jun 2016 02:49, "Christophe Bornet" notifications@github.com wrote:

So they want awesome features but no code. Like in magic ?

If you're ok, I'll do it as a silent option (no question asked) so that it
can be configured in jhipster-uml/JDL

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#3751 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABDlFz0uLEcBlCIxvSMcCWh_f7FZFBegks5qOYOdgaJpZM4I7K6u
.

[deepu105]

may be you can add --fluentMethods as an option to sub generator as well when creating entities by hand.
dont forget to PR the JDL project as well

[cbornet]

Yes, I'll add the command line option.

[cbornet]

Done. I'll add it to the JDL once this is merged.

[jdubois]

Yes of course. I'm the one who broke it anyway!

[jdubois]

I've changed my mind after reading the code: I'm keeping the "fluent-method" option, for backward compatibility (and if people are unhappy with this), but I'm putting it at "true" by default.
It's a very simple switch, so no big maintenance cost in the end.

[jdubois]

I found a couple of small issues, I'll finish this tomorrow!

[jdubois]

I'm going to merge this, just a couple of issues I'd like to share before.

There was an "add_" method for adding an entity to a collection (for one-to-many and many-to-many relationships), so I added also a "remove_" method:

• Those "add_" and "remove_" methods forced me to add a new question in many-to-many relationships, when the entity is the owner -> you need to know the name of the relationship in the other entity. We were already asking this question for other use cases, and this is the "otherEntityRelationshipName" parameter.
• Those 2 methods also kind of force to have bi-directional relationships -> if you don't want the bi-directional relationship, there is a couple of more lines to delete

So for me this needs more testing and practice.

[cbornet]

I totally forgot about unidirectional relationships ! We should detect them and not set the remote field for these.

[jdubois]

Yes and I broke the build because of this... There must be one edge case I missed.

This is quite annoying, I'm wondering if that's the right approach. I'm first correcting the build, but then we must have a look at this, and its consequences.

[jdubois]

In fact most of the errors where in our .json test files, which where not correct :-)

I've also added a ticket to JHipster Core so this is included in the JDL!

[cbornet]

@jdubois I wanted to have a look at unidirectional relationships and I couldn't see any issue.
For unidir ManyToOne, the method is not generated.
And unidir OneToMany are not allowed in jhipster.
Am I missing something ?

[jdubois]

@cbornet I didn't try it after my last comment about it, and the code has changed since, so maybe there isn't any issue after all. If you tried and it works, that's perfect!

[cbornet]

Ok. I'll check when I come back.
Also I think there might still be an issue in ManyToMany : in the adder method we should add to the remote collection instead of calling its setter. Can someone have a look ?

[cbornet]

Oh ! You've already fixed that @jdubois ! 👍 😄

[jdubois]

I don't remember everything I've fixed :-)
But yes there was some serious work on this part! And I'm still not 100% sure it's perfect

[ghost]

Check project Lombok https://projectlombok.org/features/index.html
This feature and more can be achieved with simple annotations

[cm325]

I was actually kind of suprised that jhipster wasn't already using lombok, seems like that's what all the cool kids are using these days ;) I personally understand the reasons against it, but I've seen it in use in lots of places-

[pascalgrimaud]

@dhaalves-uel @cm325 : you can look at older issues like #398 #907 #1334 #2734 #3476

[deepu105]

@jdubois @cbornet sorry to beat around the dead horse but just wanted to ask. Since its bean a while that we did this and since no body complains should we make fluent setters the default with public Bar setFoo() syntax and remove the duplicate methods?
I was looking at this and thought it would have been nice to have fluent style everywhere

[jdubois]

I agree with you, but we don't know what people use: maybe most users still use setters... So, in doubt, I would keep them, even if I agree they should be removed

[deepu105]

@jdubois we are tracking it insight.track('entity/fluentMethods', this.fluentMethods); so may be we can check if anybody has turned it off and also for user class and other stuff we can start using fluent style.
For those who are using setters already they wouldn't see any difference if we start returning the object in that setter I guess

[jdubois]

I just checked, we have 0,01% of people not using fluent methods... If it doesn't do any harm (I haven't checked), let's remove this, OK.
But can we first do the Angular 4 code? This is less important

[cbornet]

Note : if we remove the setters then we don't respect the javabean contract anymore. Is it not an issue ? (Hibernate and Jackson don't rely on javabean. I think neither does mapstruct)

[cbornet]

What we can do is to have fluent setters by default that you can deactivate with --no-fluent-methods. And refactor the User classes.
@jdubois I can't really help on Angular (me back-end type 😅 ) so I can take this if you want.

[cbornet]

We also need to cover the UserMapper : our coverage decreased 😭

[jdubois]

@cbornet aren't they already fluent by default?

[cbornet]

@jdubois indeed !

[deepu105]

ofcourse I wasnt suggesting we do it now. I just wanted to mention so that I dont forget. I mean we could create only fluent by default and the flag can be used to create non fluent if people want and for user classes we can have only fluent as they are jhipster domain classes. Anyways its not urgent at all.