Add an entity option to use fluent setters

[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 :smile: ) 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 https://github.com/jhipster/generator-jhipster/pull/3751#issuecomment-227840839, or mute the thread https://github.com/notifications/unsubscribe/ABDlFz0uLEcBlCIxvSMcCWh_f7FZFBegks5qOYOdgaJpZM4I7K6u .

[cbornet]

Yes, I'll add the command line option.

[cbornet]

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

[deepu105]

:+1:

[jdubois]

Then don't forget to document it on jhipster.github.io -> this is indeed a very nice option if you know what it does, and I'm sure a lot of people will like it And can you also add "insight" statistics -> if a lot of people use it, we can then convert it to a new question

[cbornet]

Yup, will do. As for the insight, I have already added this. Is there something missing ?

Also what should be the semantic for the JDL ? fluent-methods A,B ?

[deepu105]

@cbornet I think it should be fluentMethods A,B follow the syntax for skipClient

[cbornet]

Also I just tried

        Foo foo = new Foo()
            .myString("test")
            .addBar(new Bar().mystr("bar1"))
            .addBar(new Bar().mystr("bar2"));
        fooRepository.save(foo);

With a CascadeType.ALL on the OneToMany and the Bar entities where added to the DB. If I don't put CascadeType.ALL, they are not. So it does add the entities automatically without the need to call save().

[cbornet]

@deepu105 noted !

[jdubois]

For CascadeType.ALL-> yes, that's cascading the "many" entities, then that's because those are not already managed

I'd rather keep this with the default: I'd rather people learn how it works, than doing a surprise to people who already knows how it works

[cbornet]

No problem, it's not a big work to edit it by hand when you need it anyway.

Note : I should have written

        fooRepository.save( new Foo()
            .myString("test")
            .addBar(new Bar().mystr("bar1"))
            .addBar(new Bar().mystr("bar2")));

:wink:

[ghahramani]

@jdubois Actually I converted all the jhipster setter to fluent style :D with no problems. I like this style it gets code much cleaner

[cbornet]

Remove "needs-discussion" label ?

[jdubois]

Yes let's do this, I agree with you!

[jdubois]

@cbornet maybe you could use those in some generated tests, so that people see how it should works? And if that's good, we could put this option as "true" by default... As we should push best practices to users, and that's definitely a good way to use the entities. My only concern is that we would need a lot of testing, to check if this works with all options (including MapStruct, Cassandra...)

[cbornet]

I have tested that javaBean reflection works correctly (both reading/writing) so if those libs use it they should be OK. At the beginning I had done it in the tests but since this is now an option, I removed it. Now, I could check the fluentMethods flag and generate the test accordingly.

[cbornet]

I have added the tests and now it's cross-tested by CI with Mongo and Cassandra. I haven't added a cross-test with mapstruct but it is easy to do if needed.

[jdubois]

No need to rush -> if we do a release tomorrow, it's a bit early to merge this anyway. But indeed I'd like to have this tested also with MapStruct. And we need to have MapStruct out of beta at some point :-)

[jdubois]

OK, this all looks good to me.

Now, 2 things:

• we need more testing -> but this can be done after merging this
• do we put this by default or not? I like the fact that we provide "best practices" and push them, then I don't know how people will react to this -> I'll do a Twitter survey and mention @cbornet

[jdubois]

And the vote has started at https://twitter.com/java_hipster/status/760504585748447232

[vladmihalcea]

Although the JPA specification forbids Java Bean conventions violation, Hibernate is not so strict. For instance, you can run this test just fine. As long as you rely only on Hibernate, you should be fine. However, if you plan on supporting other JPA providers, the Fluent API might not work, so keep that in mind as well.

[jdubois]

Thanks a lot @vladmihalcea !!! I didn't know it was a JPA spec violation, and in that case that's quite a big issue to me. It would also go against our Policy 1.

[vladmihalcea]

It's the 2.2 Persistent Fields and Properties section of the Java Persistence 2.1 spec that says:

In this case, for every persistent property property of type T of the entity, there is a getter method, getProperty, and setter method setProperty. For boolean properties, isProperty may be used as an alternative name for the getter method.[2]

For single-valued persistent properties, these method signatures are:

• T getProperty()
• void setProperty(T t)

Now, the spec needs to be generic in order to allow interoperability. In reality, JPA implementations might relax some requirements to provide a better developer experience.

Since you want to support any JPA provider, you might want to contact all major implementation and validate that the Fluent API is not breaking anything.

[jdubois]

Thanks again @vladmihalcea No we don't intend to support all JPA providers (just Hibernate, in fact), but we try to follow the "standard" usage of each technology (and so follow JPA, here). I'll wait until the vote is over: at the moment we have 70% votes for the "fluent setters"!

[cbornet]

The PR doesn't remove the java bean methods and it must not as some frameworks may use java bean reflection. The fluent methods are added methods with different signature so they don't disturb the reflection.

[vladmihalcea]

But you cannot have two setter methods whose signature only differ in the return valuue type. How are you going to address that?

[cbornet]

They differ in name : void setBar() vs Foo bar().

[vladmihalcea]

Seems legit.

[jdubois]

So the vote is over, 116 votes, 72% for the "fluent setters". Besides, we had a nice blog entry by @vladmihalcea about (see it here), and this was even tweeted by the official Java account (see it here). So an overall very clear vote for this.

• let's do this
• let's not put this as an option: it adds more maintenance cost, and there is no big issue in doing this (except extra code for non-hipsters who want to use the classical setters)